From 45577a1f4c84eca059453908220cbdc7e1d34f93 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Ho=C5=99e=C5=88ovsk=C3=BD?= Date: Sun, 26 Dec 2021 22:10:20 +0100 Subject: [PATCH] Refactor implementation of case-insensitivity in tags By not materializing the lower cased tags ahead of time, we save allocations at the cost of worsened performance when comparing two tags. Since there are rarely many tags, and commonly they are not compared even if present, this is almost always a win. The new implementation also improves the robustness of the code responsible for handling tags in a case-insensitive manner. --- examples/210-Evt-EventListeners.cpp | 2 +- src/catch2/catch_test_case_info.cpp | 27 ++++++++----------- src/catch2/catch_test_case_info.hpp | 18 +++++++++---- src/catch2/catch_test_spec.cpp | 10 +++---- src/catch2/internal/catch_list.cpp | 7 ++--- .../Baselines/compact.sw.approved.txt | 2 +- .../Baselines/console.sw.approved.txt | 4 +-- tests/SelfTest/Baselines/tap.sw.approved.txt | 2 +- tests/SelfTest/Baselines/xml.sw.approved.txt | 4 +-- .../SelfTest/IntrospectiveTests/Tag.tests.cpp | 11 +++----- 10 files changed, 43 insertions(+), 44 deletions(-) diff --git a/examples/210-Evt-EventListeners.cpp b/examples/210-Evt-EventListeners.cpp index f6eff642..5468112e 100644 --- a/examples/210-Evt-EventListeners.cpp +++ b/examples/210-Evt-EventListeners.cpp @@ -22,7 +22,7 @@ std::string ws(int const level) { } std::ostream& operator<<(std::ostream& out, Catch::Tag t) { - return out << "original: " << t.original << "lower cased: " << t.lowerCased; + return out << "original: " << t.original; } template< typename T > diff --git a/src/catch2/catch_test_case_info.cpp b/src/catch2/catch_test_case_info.cpp index 7dd88c2d..83cfcc8e 100644 --- a/src/catch2/catch_test_case_info.cpp +++ b/src/catch2/catch_test_case_info.cpp @@ -10,6 +10,7 @@ #include #include #include +#include #include #include @@ -103,8 +104,13 @@ namespace Catch { } } // end unnamed namespace - bool operator<( Tag const& lhs, Tag const& rhs ) { - return lhs.original < rhs.original; + bool operator<( Tag const& lhs, Tag const& rhs ) { + Detail::CaseInsensitiveLess cmp; + return cmp( lhs.original, rhs.original ); + } + bool operator==( Tag const& lhs, Tag const& rhs ) { + Detail::CaseInsensitiveEqualTo cmp; + return cmp( lhs.original, rhs.original ); } Detail::unique_ptr @@ -126,7 +132,6 @@ namespace Catch { // (including optional hidden tag and filename tag) auto requiredSize = originalTags.size() + sizeOfExtraTags(_lineInfo.file); backingTags.reserve(requiredSize); - backingLCaseTags.reserve(requiredSize); // We cannot copy the tags directly, as we need to normalize // some tags, so that [.foo] is copied as [.][foo]. @@ -172,9 +177,8 @@ namespace Catch { } // Sort and prepare tags - toLowerInPlace(backingLCaseTags); - std::sort(begin(tags), end(tags), [](Tag lhs, Tag rhs) { return lhs.lowerCased < rhs.lowerCased; }); - tags.erase(std::unique(begin(tags), end(tags), [](Tag lhs, Tag rhs) {return lhs.lowerCased == rhs.lowerCased; }), + std::sort(begin(tags), end(tags)); + tags.erase(std::unique(begin(tags), end(tags)), end(tags)); } @@ -195,9 +199,6 @@ namespace Catch { std::string combined("#"); combined += extractFilenamePart(lineInfo.file); internalAppendTag(combined); - // TBD: Running this over all tags again is inefficient, but - // simple enough. In practice, the overhead is small enough. - toLowerInPlace(backingLCaseTags); } std::string TestCaseInfo::tagsAsString() const { @@ -223,13 +224,7 @@ namespace Catch { backingTags += tagStr; const auto backingEnd = backingTags.size(); backingTags += ']'; - backingLCaseTags += '['; - // We append the tag to the lower-case backing storage as-is, - // because we will perform the lower casing later, in bulk - backingLCaseTags += tagStr; - backingLCaseTags += ']'; - tags.emplace_back(StringRef(backingTags.c_str() + backingStart, backingEnd - backingStart), - StringRef(backingLCaseTags.c_str() + backingStart, backingEnd - backingStart)); + tags.emplace_back(StringRef(backingTags.c_str() + backingStart, backingEnd - backingStart)); } bool operator<( TestCaseInfo const& lhs, TestCaseInfo const& rhs ) { diff --git a/src/catch2/catch_test_case_info.hpp b/src/catch2/catch_test_case_info.hpp index cae3b45a..3ea656f6 100644 --- a/src/catch2/catch_test_case_info.hpp +++ b/src/catch2/catch_test_case_info.hpp @@ -25,13 +25,21 @@ namespace Catch { + /** + * A **view** of a tag string that provides case insensitive comparisons + * + * Note that in Catch2 internals, the square brackets around tags are + * not a part of tag's representation, so e.g. "[cool-tag]" is represented + * as "cool-tag" internally. + */ struct Tag { - Tag(StringRef original_, StringRef lowerCased_): - original(original_), lowerCased(lowerCased_) + constexpr Tag(StringRef original_): + original(original_) {} - StringRef original, lowerCased; + StringRef original; - friend bool operator<( Tag const& lhs, Tag const& rhs ); + friend bool operator< ( Tag const& lhs, Tag const& rhs ); + friend bool operator==( Tag const& lhs, Tag const& rhs ); }; struct ITestInvoker; @@ -79,7 +87,7 @@ namespace Catch { std::string name; StringRef className; private: - std::string backingTags, backingLCaseTags; + std::string backingTags; // Internally we copy tags to the backing storage and then add // refs to this storage to the tags vector. void internalAppendTag(StringRef tagString); diff --git a/src/catch2/catch_test_spec.cpp b/src/catch2/catch_test_spec.cpp index 4c59695d..a3235d43 100644 --- a/src/catch2/catch_test_spec.cpp +++ b/src/catch2/catch_test_spec.cpp @@ -38,15 +38,13 @@ namespace Catch { TestSpec::TagPattern::TagPattern( std::string const& tag, std::string const& filterString ) : Pattern( filterString ) - , m_tag( toLower( tag ) ) + , m_tag( tag ) {} bool TestSpec::TagPattern::matches( TestCaseInfo const& testCase ) const { - return std::find_if(begin(testCase.tags), - end(testCase.tags), - [&](Tag const& tag) { - return tag.lowerCased == m_tag; - }) != end(testCase.tags); + return std::find( begin( testCase.tags ), + end( testCase.tags ), + Tag( m_tag ) ) != end( testCase.tags ); } bool TestSpec::Filter::matches( TestCaseInfo const& testCase ) const { diff --git a/src/catch2/internal/catch_list.cpp b/src/catch2/internal/catch_list.cpp index 20613c70..6d99dfb1 100644 --- a/src/catch2/internal/catch_list.cpp +++ b/src/catch2/internal/catch_list.cpp @@ -13,6 +13,7 @@ #include #include #include +#include #include #include @@ -32,12 +33,12 @@ namespace Catch { auto const& testSpec = config.testSpec(); std::vector matchedTestCases = filterTests(getAllTestCasesSorted(config), testSpec, config); - std::map tagCounts; + std::map tagCounts; for (auto const& testCase : matchedTestCases) { for (auto const& tagName : testCase.getTestCaseInfo().tags) { - auto it = tagCounts.find(tagName.lowerCased); + auto it = tagCounts.find(tagName.original); if (it == tagCounts.end()) - it = tagCounts.insert(std::make_pair(tagName.lowerCased, TagInfo())).first; + it = tagCounts.insert(std::make_pair(tagName.original, TagInfo())).first; it->second.add(tagName.original); } } diff --git a/tests/SelfTest/Baselines/compact.sw.approved.txt b/tests/SelfTest/Baselines/compact.sw.approved.txt index da97ee82..f58324f4 100644 --- a/tests/SelfTest/Baselines/compact.sw.approved.txt +++ b/tests/SelfTest/Baselines/compact.sw.approved.txt @@ -2281,7 +2281,7 @@ InternalBenchmark.tests.cpp:: passed: Timing.result == Timing.itera InternalBenchmark.tests.cpp:: passed: Timing.iterations >= time.count() for: 128 >= 100 Misc.tests.cpp:: failed: false with 1 message: '3' Message.tests.cpp:: failed: false with 2 messages: 'hi' and 'i := 7' -Tag.tests.cpp:: passed: tags, VectorContains("magic-tag"_catch_sr) && VectorContains("."_catch_sr) for: { ., magic-tag } ( Contains: magic-tag and Contains: . ) +Tag.tests.cpp:: passed: testcase.tags, VectorContains( Tag( "magic-tag" ) ) && VectorContains( Tag( "."_catch_sr ) ) for: { {?}, {?} } ( Contains: {?} and Contains: {?} ) StringManip.tests.cpp:: passed: splitStringRef("", ','), Equals(std::vector()) for: { } Equals: { } StringManip.tests.cpp:: passed: splitStringRef("abc", ','), Equals(std::vector{"abc"}) for: { abc } Equals: { abc } StringManip.tests.cpp:: passed: splitStringRef("abc,def", ','), Equals(std::vector{"abc", "def"}) for: { abc, def } Equals: { abc, def } diff --git a/tests/SelfTest/Baselines/console.sw.approved.txt b/tests/SelfTest/Baselines/console.sw.approved.txt index 7d637d7e..25a85bce 100644 --- a/tests/SelfTest/Baselines/console.sw.approved.txt +++ b/tests/SelfTest/Baselines/console.sw.approved.txt @@ -16278,9 +16278,9 @@ Tag.tests.cpp: ............................................................................... Tag.tests.cpp:: PASSED: - REQUIRE_THAT( tags, VectorContains("magic-tag"_catch_sr) && VectorContains("."_catch_sr) ) + REQUIRE_THAT( testcase.tags, VectorContains( Tag( "magic-tag" ) ) && VectorContains( Tag( "."_catch_sr ) ) ) with expansion: - { ., magic-tag } ( Contains: magic-tag and Contains: . ) + { {?}, {?} } ( Contains: {?} and Contains: {?} ) ------------------------------------------------------------------------------- splitString diff --git a/tests/SelfTest/Baselines/tap.sw.approved.txt b/tests/SelfTest/Baselines/tap.sw.approved.txt index a02d3417..746f2599 100644 --- a/tests/SelfTest/Baselines/tap.sw.approved.txt +++ b/tests/SelfTest/Baselines/tap.sw.approved.txt @@ -4119,7 +4119,7 @@ not ok {test-number} - false with 1 message: '3' # sends information to INFO not ok {test-number} - false with 2 messages: 'hi' and 'i := 7' # shortened hide tags are split apart -ok {test-number} - tags, VectorContains("magic-tag"_catch_sr) && VectorContains("."_catch_sr) for: { ., magic-tag } ( Contains: magic-tag and Contains: . ) +ok {test-number} - testcase.tags, VectorContains( Tag( "magic-tag" ) ) && VectorContains( Tag( "."_catch_sr ) ) for: { {?}, {?} } ( Contains: {?} and Contains: {?} ) # splitString ok {test-number} - splitStringRef("", ','), Equals(std::vector()) for: { } Equals: { } # splitString diff --git a/tests/SelfTest/Baselines/xml.sw.approved.txt b/tests/SelfTest/Baselines/xml.sw.approved.txt index 4602696b..b9bb761e 100644 --- a/tests/SelfTest/Baselines/xml.sw.approved.txt +++ b/tests/SelfTest/Baselines/xml.sw.approved.txt @@ -19158,10 +19158,10 @@ loose text artifact - tags, VectorContains("magic-tag"_catch_sr) && VectorContains("."_catch_sr) + testcase.tags, VectorContains( Tag( "magic-tag" ) ) && VectorContains( Tag( "."_catch_sr ) ) - { ., magic-tag } ( Contains: magic-tag and Contains: . ) + { {?}, {?} } ( Contains: {?} and Contains: {?} ) diff --git a/tests/SelfTest/IntrospectiveTests/Tag.tests.cpp b/tests/SelfTest/IntrospectiveTests/Tag.tests.cpp index 2feffa44..468a6792 100644 --- a/tests/SelfTest/IntrospectiveTests/Tag.tests.cpp +++ b/tests/SelfTest/IntrospectiveTests/Tag.tests.cpp @@ -44,15 +44,12 @@ constexpr Catch::SourceLineInfo dummySourceLineInfo = CATCH_INTERNAL_LINEINFO; TEST_CASE("shortened hide tags are split apart", "[tags]") { using Catch::StringRef; + using Catch::Tag; using Catch::Matchers::VectorContains; - Catch::TestCaseInfo testcase("", {"fake test name", "[.magic-tag]"}, dummySourceLineInfo); - // Extract parsed tags into strings - std::vector tags; - for (auto const& tag : testcase.tags) { - tags.push_back(tag.lowerCased); - } - REQUIRE_THAT(tags, VectorContains("magic-tag"_catch_sr) && VectorContains("."_catch_sr)); + Catch::TestCaseInfo testcase("", {"fake test name", "[.magic-tag]"}, dummySourceLineInfo); + REQUIRE_THAT( testcase.tags, VectorContains( Tag( "magic-tag" ) ) + && VectorContains( Tag( "."_catch_sr ) ) ); } TEST_CASE("tags with dots in later positions are not parsed as hidden", "[tags]") {