ColourGuard is no longer constructed engaged

Forcing it to be engaged explicitly, either via `op<<`, or by
`ColourGuard::engage`, fixes an issue with multiple `ColourGuard`s
being constructed in a single expression. Because the construction
of the `ColourGuard` instances can happen in arbitrary order,
colours would be applied in arbitrary order too. However, a chain
of `op<<`s has strict call orders, fixing this issue.

src/catch2/catch_session.cpp

@@ -153,7 +153,7 @@ namespace Catch {
             m_startupExceptions = true;
             auto errStream = makeStream( "%stderr" );
             auto colourImpl = makeColourImpl( &config(), errStream.get() );
-            auto guard = colourImpl->startColour( Colour::Red );
+            auto guard = colourImpl->guardColour( Colour::Red );
             errStream->stream() << "Errors occurred during startup!" << '\n';
             // iterate over all exceptions and notify user
             for ( const auto& ex_ptr : exceptions ) {
@@ -200,7 +200,7 @@ namespace Catch {
             auto colour = makeColourImpl( &config(), errStream.get() );
 
             errStream->stream()
-                << colour->startColour( Colour::Red )
+                << colour->guardColour( Colour::Red )
                 << "\nError(s) in input:\n"
                 << TextFlow::Column( result.errorMessage() ).indent( 2 )
                 << "\n\n";

src/catch2/internal/catch_console_colour.cpp

@@ -27,7 +27,7 @@ namespace Catch {
 
     ColourImpl::~ColourImpl() = default;
 
-    ColourImpl::ColourGuard ColourImpl::startColour( Colour::Code colourCode ) {
+    ColourImpl::ColourGuard ColourImpl::guardColour( Colour::Code colourCode ) {
         return ColourGuard(colourCode, this );
     }
 
@@ -45,27 +45,53 @@ namespace Catch {
 
     } // namespace
 
-    ColourImpl::ColourGuard::ColourGuard( Colour::Code code,
-                              ColourImpl const* colour ):
-        m_colourImpl( colour ) {
-        m_colourImpl->use( code );
+    void ColourImpl::ColourGuard::engageImpl( std::ostream& stream ) {
+        assert( &stream == &m_colourImpl->m_stream->stream() &&
+                "Engaging colour guard for different stream than used by the "
+                "parent colour implementation" );
+        static_cast<void>( stream );
+
+        m_engaged = true;
+        m_colourImpl->use( m_code );
     }
-    ColourImpl::ColourGuard::ColourGuard( ColourGuard&& rhs ):
-        m_colourImpl( rhs.m_colourImpl ) {
-        rhs.m_moved = true;
+
+    ColourImpl::ColourGuard::ColourGuard( Colour::Code code,
+                                          ColourImpl const* colour ):
+        m_colourImpl( colour ), m_code( code ) {
+    }
+    ColourImpl::ColourGuard::ColourGuard( ColourGuard&& rhs ) noexcept:
+        m_colourImpl( rhs.m_colourImpl ),
+        m_code( rhs.m_code ),
+        m_engaged( rhs.m_engaged ) {
+        rhs.m_engaged = false;
     }
     ColourImpl::ColourGuard&
-    ColourImpl::ColourGuard::operator=( ColourGuard&& rhs ) {
-        m_colourImpl = rhs.m_colourImpl;
-        rhs.m_moved = true;
+    ColourImpl::ColourGuard::operator=( ColourGuard&& rhs ) noexcept {
+        using std::swap;
+        swap( m_colourImpl, rhs.m_colourImpl );
+        swap( m_code, rhs.m_code );
+        swap( m_engaged, rhs.m_engaged );
+
         return *this;
     }
     ColourImpl::ColourGuard::~ColourGuard() {
-        if ( !m_moved ) {
+        if ( m_engaged ) {
             m_colourImpl->use( Colour::None );
         }
     }
 
+    ColourImpl::ColourGuard&
+    ColourImpl::ColourGuard::engage( std::ostream& stream ) & {
+        engageImpl( stream );
+        return *this;
+    }
+
+    ColourImpl::ColourGuard&&
+    ColourImpl::ColourGuard::engage( std::ostream& stream ) && {
+        engageImpl( stream );
+        return CATCH_MOVE(*this);
+    }
+
 } // namespace Catch
 
 #if !defined( CATCH_CONFIG_COLOUR_NONE ) && !defined( CATCH_CONFIG_COLOUR_WINDOWS ) && !defined( CATCH_CONFIG_COLOUR_ANSI )

src/catch2/internal/catch_console_colour.hpp

@@ -62,25 +62,66 @@ namespace Catch {
     public:
         ColourImpl( IStream const* stream ): m_stream( stream ) {}
 
+        //! RAII wrapper around writing specific colour of text using specific
+        //! colour impl into a stream.
         class ColourGuard {
             ColourImpl const* m_colourImpl;
-            bool m_moved = false;
-            friend std::ostream& operator<<(std::ostream& lhs,
-                                            ColourGuard const&) {
-                return lhs;
-            }
+            Colour::Code m_code;
+            bool m_engaged = false;
+
         public:
+            //! Does **not** engage the guard/start the colour
             ColourGuard( Colour::Code code,
                          ColourImpl const* colour );
+
             ColourGuard( ColourGuard const& rhs ) = delete;
             ColourGuard& operator=( ColourGuard const& rhs ) = delete;
-            ColourGuard( ColourGuard&& rhs );
-            ColourGuard& operator=( ColourGuard&& rhs );
+
+            ColourGuard( ColourGuard&& rhs ) noexcept;
+            ColourGuard& operator=( ColourGuard&& rhs ) noexcept;
+
+            //! Removes colour _if_ the guard was engaged
             ~ColourGuard();
+
+            /**
+             * Explicitly engages colour for given stream.
+             *
+             * The API based on operator<< should be preferred.
+             */
+            ColourGuard& engage( std::ostream& stream ) &;
+            /**
+             * Explicitly engages colour for given stream.
+             *
+             * The API based on operator<< should be preferred.
+             */
+            ColourGuard&& engage( std::ostream& stream ) &&;
+
+        private:
+            //! Engages the guard and starts using colour
+            friend std::ostream& operator<<( std::ostream& lhs,
+                                             ColourGuard& guard ) {
+                guard.engageImpl( lhs );
+                return lhs;
+            }
+            //! Engages the guard and starts using colour
+            friend std::ostream& operator<<( std::ostream& lhs,
+                                            ColourGuard&& guard) {
+                guard.engageImpl( lhs );
+                return lhs;
+            }
+
+            void engageImpl( std::ostream& stream );
+
         };
 
         virtual ~ColourImpl(); // = default
-        ColourGuard startColour( Colour::Code colourCode );
+        /**
+         * Creates a guard object for given colour and this colour impl
+         *
+         * **Important:**
+         * the guard starts disengaged, and has to be engaged explicitly.
+         */
+        ColourGuard guardColour( Colour::Code colourCode );
 
     private:
         virtual void use( Colour::Code colourCode ) const = 0;

src/catch2/reporters/catch_reporter_combined_tu.cpp

@@ -194,7 +194,7 @@ namespace Catch {
             Colour::Code colour = testCaseInfo.isHidden()
                 ? Colour::SecondaryText
                 : Colour::None;
-            auto colourGuard = streamColour->startColour( colour );
+            auto colourGuard = streamColour->guardColour( colour ).engage( out );
 
             out << TextFlow::Column(testCaseInfo.name).initialIndent(2).indent(4) << '\n';
             if (verbosity >= Verbosity::High) {

src/catch2/reporters/catch_reporter_compact.cpp

@@ -56,7 +56,7 @@ void printTotals(std::ostream& out, const Totals& totals, ColourImpl* colourImpl
     if (totals.testCases.total() == 0) {
         out << "No tests ran.";
     } else if (totals.testCases.failed == totals.testCases.total()) {
-        auto guard = colourImpl->startColour( Colour::ResultError );
+        auto guard = colourImpl->guardColour( Colour::ResultError ).engage( out );
         const StringRef qualify_assertions_failed =
             totals.assertions.failed == totals.assertions.total() ?
             bothOrAll(totals.assertions.failed) : StringRef{};
@@ -71,11 +71,11 @@ void printTotals(std::ostream& out, const Totals& totals, ColourImpl* colourImpl
             << pluralise(totals.testCases.total(), "test case"_sr)
             << " (no assertions).";
     } else if (totals.assertions.failed) {
-        out << colourImpl->startColour( Colour::ResultError ) <<
+        out << colourImpl->guardColour( Colour::ResultError ) <<
             "Failed " << pluralise(totals.testCases.failed, "test case"_sr) << ", "
             "failed " << pluralise(totals.assertions.failed, "assertion"_sr) << '.';
     } else {
-        out << colourImpl->startColour( Colour::ResultSuccess ) <<
+        out << colourImpl->guardColour( Colour::ResultSuccess ) <<
             "Passed " << bothOrAll(totals.testCases.passed)
             << pluralise(totals.testCases.passed, "test case"_sr) <<
             " with " << pluralise(totals.assertions.passed, "assertion"_sr) << '.';
@@ -166,13 +166,13 @@ public:
 
 private:
     void printSourceInfo() const {
-        stream << colourImpl->startColour( Colour::FileName )
+        stream << colourImpl->guardColour( Colour::FileName )
                << result.getSourceInfo() << ':';
     }
 
     void printResultType(Colour::Code colour, StringRef passOrFail) const {
         if (!passOrFail.empty()) {
-            stream << colourImpl->startColour(colour) << ' ' << passOrFail;
+            stream << colourImpl->guardColour(colour) << ' ' << passOrFail;
             stream << ':';
         }
     }
@@ -185,7 +185,7 @@ private:
         if (result.hasExpression()) {
             stream << ';';
             {
-                stream << colourImpl->startColour(compactDimColour) << " expression was:";
+                stream << colourImpl->guardColour(compactDimColour) << " expression was:";
             }
             printOriginalExpression();
         }
@@ -199,7 +199,7 @@ private:
 
     void printReconstructedExpression() const {
         if (result.hasExpandedExpression()) {
-            stream << colourImpl->startColour(compactDimColour) << " for: ";
+            stream << colourImpl->guardColour(compactDimColour) << " for: ";
             stream << result.getExpandedExpression();
         }
     }
@@ -218,7 +218,7 @@ private:
         const auto itEnd = messages.cend();
         const auto N = static_cast<std::size_t>(std::distance(itMessage, itEnd));
 
-        stream << colourImpl->startColour( colour ) << " with "
+        stream << colourImpl->guardColour( colour ) << " with "
                << pluralise( N, "message"_sr ) << ':';
 
         while (itMessage != itEnd) {
@@ -226,7 +226,7 @@ private:
             if (printInfoMessages || itMessage->type != ResultWas::Info) {
                 printMessage();
                 if (itMessage != itEnd) {
-                    stream << colourImpl->startColour(compactDimColour) << " and";
+                    stream << colourImpl->guardColour(compactDimColour) << " and";
                 }
                 continue;
             }

src/catch2/reporters/catch_reporter_console.cpp

@@ -135,19 +135,19 @@ public:
 private:
     void printResultType() const {
         if (!passOrFail.empty()) {
-            stream << colourImpl->startColour(colour) << passOrFail << ":\n";
+            stream << colourImpl->guardColour(colour) << passOrFail << ":\n";
         }
     }
     void printOriginalExpression() const {
         if (result.hasExpression()) {
-            stream << colourImpl->startColour( Colour::OriginalExpression )
+            stream << colourImpl->guardColour( Colour::OriginalExpression )
                    << "  " << result.getExpressionInMacro() << '\n';
         }
     }
     void printReconstructedExpression() const {
         if (result.hasExpandedExpression()) {
             stream << "with expansion:\n";
-            stream << colourImpl->startColour( Colour::ReconstructedExpression )
+            stream << colourImpl->guardColour( Colour::ReconstructedExpression )
                    << TextFlow::Column( result.getExpandedExpression() )
                           .indent( 2 )
                    << '\n';
@@ -163,7 +163,7 @@ private:
         }
     }
     void printSourceInfo() const {
-        stream << colourImpl->startColour( Colour::FileName )
+        stream << colourImpl->guardColour( Colour::FileName )
                << result.getSourceInfo() << ": ";
     }
 
@@ -418,7 +418,8 @@ void ConsoleReporter::sectionEnded(SectionStats const& _sectionStats) {
     m_tablePrinter->close();
     if (_sectionStats.missingAssertions) {
         lazyPrint();
-        auto guard = m_colour->startColour( Colour::ResultError );
+        auto guard =
+            m_colour->guardColour( Colour::ResultError ).engage( m_stream );
         if (m_sectionStack.size() > 1)
             m_stream << "\nNo assertions in section";
         else
@@ -476,7 +477,7 @@ void ConsoleReporter::benchmarkEnded(BenchmarkStats<> const& stats) {
 }
 
 void ConsoleReporter::benchmarkFailed( StringRef error ) {
-    auto guard = m_colour->startColour( Colour::Red );
+    auto guard = m_colour->guardColour( Colour::Red ).engage( m_stream );
     (*m_tablePrinter)
         << "Benchmark failed (" << error << ')'
         << ColumnBreak() << RowBreak();
@@ -517,7 +518,7 @@ void ConsoleReporter::lazyPrintWithoutClosingBenchmarkTable() {
 void ConsoleReporter::lazyPrintRunInfo() {
     m_stream << '\n'
              << lineOfChars( '~' ) << '\n'
-             << m_colour->startColour( Colour::SecondaryText )
+             << m_colour->guardColour( Colour::SecondaryText )
              << currentTestRunInfo.name << " is a Catch2 v" << libraryVersion()
              << " host application.\n"
              << "Run with -? for options\n\n"
@@ -530,7 +531,7 @@ void ConsoleReporter::printTestCaseAndSectionHeader() {
     printOpenHeader(currentTestCaseInfo->name);
 
     if (m_sectionStack.size() > 1) {
-        auto guard = m_colour->startColour( Colour::Headers );
+        auto guard = m_colour->guardColour( Colour::Headers ).engage( m_stream );
 
         auto
             it = m_sectionStack.begin() + 1, // Skip first section (test case)
@@ -543,7 +544,7 @@ void ConsoleReporter::printTestCaseAndSectionHeader() {
 
 
     m_stream << lineOfChars( '-' ) << '\n'
-             << m_colour->startColour( Colour::FileName ) << lineInfo << '\n'
+             << m_colour->guardColour( Colour::FileName ) << lineInfo << '\n'
              << lineOfChars( '.' ) << "\n\n"
              << std::flush;
 }
@@ -555,7 +556,7 @@ void ConsoleReporter::printClosedHeader(std::string const& _name) {
 void ConsoleReporter::printOpenHeader(std::string const& _name) {
     m_stream << lineOfChars('-') << '\n';
     {
-        auto guard = m_colour->startColour( Colour::Headers );
+        auto guard = m_colour->guardColour( Colour::Headers ).engage( m_stream );
         printHeaderString(_name);
     }
 }
@@ -620,10 +621,10 @@ struct SummaryColumn {
 
 void ConsoleReporter::printTotals( Totals const& totals ) {
     if (totals.testCases.total() == 0) {
-        m_stream << m_colour->startColour( Colour::Warning )
+        m_stream << m_colour->guardColour( Colour::Warning )
                  << "No tests ran\n";
     } else if (totals.assertions.total() > 0 && totals.testCases.allPassed()) {
-        m_stream << m_colour->startColour( Colour::ResultSuccess )
+        m_stream << m_colour->guardColour( Colour::ResultSuccess )
                  << "All tests passed";
         m_stream << " ("
             << pluralise(totals.assertions.passed, "assertion"_sr) << " in "
@@ -657,12 +658,12 @@ void ConsoleReporter::printSummaryRow(StringRef label, std::vector<SummaryColumn
             if ( value != "0" ) {
                 m_stream << value;
             } else {
-                m_stream << m_colour->startColour( Colour::Warning )
+                m_stream << m_colour->guardColour( Colour::Warning )
                          << "- none -";
             }
         } else if (value != "0") {
-            m_stream << m_colour->startColour( Colour::LightGrey ) << " | ";
-            m_stream << m_colour->startColour( col.colour ) << value << ' '
+            m_stream << m_colour->guardColour( Colour::LightGrey ) << " | "
+                     << m_colour->guardColour( col.colour ) << value << ' '
                      << col.label;
         }
     }
@@ -679,19 +680,19 @@ void ConsoleReporter::printTotalsDivider(Totals const& totals) {
         while (failedRatio + failedButOkRatio + passedRatio > CATCH_CONFIG_CONSOLE_WIDTH - 1)
             findMax(failedRatio, failedButOkRatio, passedRatio)--;
 
-        m_stream << m_colour->startColour( Colour::Error )
+        m_stream << m_colour->guardColour( Colour::Error )
                  << std::string( failedRatio, '=' )
-                 << m_colour->startColour( Colour::ResultExpectedFailure )
+                 << m_colour->guardColour( Colour::ResultExpectedFailure )
                  << std::string( failedButOkRatio, '=' );
         if ( totals.testCases.allPassed() ) {
-            m_stream << m_colour->startColour( Colour::ResultSuccess )
+            m_stream << m_colour->guardColour( Colour::ResultSuccess )
                      << std::string( passedRatio, '=' );
         } else {
-            m_stream << m_colour->startColour( Colour::Success )
+            m_stream << m_colour->guardColour( Colour::Success )
                      << std::string( passedRatio, '=' );
         }
     } else {
-        m_stream << m_colour->startColour( Colour::Warning )
+        m_stream << m_colour->guardColour( Colour::Warning )
                  << std::string( CATCH_CONFIG_CONSOLE_WIDTH - 1, '=' );
     }
     m_stream << '\n';
@@ -702,7 +703,7 @@ void ConsoleReporter::printSummaryDivider() {
 
 void ConsoleReporter::printTestFilters() {
     if (m_config->testSpec().hasFilters()) {
-        m_stream << m_colour->startColour( Colour::BrightYellow ) << "Filters: "
+        m_stream << m_colour->guardColour( Colour::BrightYellow ) << "Filters: "
                  << serializeFilters( m_config->getTestsOrTags() ) << '\n';
     }
 }

src/catch2/reporters/catch_reporter_tap.cpp

@@ -107,7 +107,7 @@ namespace Catch {
 
         private:
             void printSourceInfo() const {
-                stream << colourImpl->startColour( tapDimColour )
+                stream << colourImpl->guardColour( tapDimColour )
                        << result.getSourceInfo() << ':';
             }
 
@@ -124,7 +124,7 @@ namespace Catch {
             void printExpressionWas() {
                 if (result.hasExpression()) {
                     stream << ';';
-                    stream << colourImpl->startColour( tapDimColour )
+                    stream << colourImpl->guardColour( tapDimColour )
                            << " expression was:";
                     printOriginalExpression();
                 }
@@ -138,7 +138,7 @@ namespace Catch {
 
             void printReconstructedExpression() const {
                 if (result.hasExpandedExpression()) {
-                    stream << colourImpl->startColour( tapDimColour ) << " for: ";
+                    stream << colourImpl->guardColour( tapDimColour ) << " for: ";
 
                     std::string expr = result.getExpandedExpression();
                     std::replace(expr.begin(), expr.end(), '\n', ' ');
@@ -162,7 +162,7 @@ namespace Catch {
                 std::vector<MessageInfo>::const_iterator itEnd = messages.end();
                 const std::size_t N = static_cast<std::size_t>(std::distance(itMessage, itEnd));
 
-                stream << colourImpl->startColour( colour ) << " with "
+                stream << colourImpl->guardColour( colour ) << " with "
                        << pluralise( N, "message"_sr ) << ':';
 
                 for (; itMessage != itEnd; ) {
@@ -170,7 +170,7 @@ namespace Catch {
                     if (printInfoMessages || itMessage->type != ResultWas::Info) {
                         stream << " '" << itMessage->message << '\'';
                         if (++itMessage != itEnd) {
-                            stream << colourImpl->startColour(tapDimColour) << " and";
+                            stream << colourImpl->guardColour(tapDimColour) << " and";
                         }
                     }
                 }