From 081a1e9abaf32a759fd08a01fcf7a65860f91a63 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Ho=C5=99e=C5=88ovsk=C3=BD?= Date: Mon, 21 Mar 2022 23:20:08 +0100 Subject: [PATCH] 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 | 4 +- src/catch2/internal/catch_console_colour.cpp | 50 ++++++++++++---- src/catch2/internal/catch_console_colour.hpp | 57 ++++++++++++++++--- .../reporters/catch_reporter_combined_tu.cpp | 2 +- .../reporters/catch_reporter_compact.cpp | 18 +++--- .../reporters/catch_reporter_console.cpp | 43 +++++++------- src/catch2/reporters/catch_reporter_tap.cpp | 10 ++-- 7 files changed, 126 insertions(+), 58 deletions(-) diff --git a/src/catch2/catch_session.cpp b/src/catch2/catch_session.cpp index 3a47974d..c685e48b 100644 --- a/src/catch2/catch_session.cpp +++ b/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"; diff --git a/src/catch2/internal/catch_console_colour.cpp b/src/catch2/internal/catch_console_colour.cpp index 7f294abc..b7b4b4fb 100644 --- a/src/catch2/internal/catch_console_colour.cpp +++ b/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( 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 ) diff --git a/src/catch2/internal/catch_console_colour.hpp b/src/catch2/internal/catch_console_colour.hpp index e6a37967..7b8f5b7a 100644 --- a/src/catch2/internal/catch_console_colour.hpp +++ b/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; diff --git a/src/catch2/reporters/catch_reporter_combined_tu.cpp b/src/catch2/reporters/catch_reporter_combined_tu.cpp index 381d0743..5b04c290 100644 --- a/src/catch2/reporters/catch_reporter_combined_tu.cpp +++ b/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) { diff --git a/src/catch2/reporters/catch_reporter_compact.cpp b/src/catch2/reporters/catch_reporter_compact.cpp index 2f0a20b0..1d3d0329 100644 --- a/src/catch2/reporters/catch_reporter_compact.cpp +++ b/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::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; } diff --git a/src/catch2/reporters/catch_reporter_console.cpp b/src/catch2/reporters/catch_reporter_console.cpp index 0d110a5a..4ad1b990 100644 --- a/src/catch2/reporters/catch_reporter_console.cpp +++ b/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::vectorstartColour( 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'; } } diff --git a/src/catch2/reporters/catch_reporter_tap.cpp b/src/catch2/reporters/catch_reporter_tap.cpp index 86a862d1..4cc91b7b 100644 --- a/src/catch2/reporters/catch_reporter_tap.cpp +++ b/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::const_iterator itEnd = messages.end(); const std::size_t N = static_cast(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"; } } }