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.
2024-11-21 21:06:11 +01:00 · 2021-12-26 22:10:20 +01:00 · 2021-12-26 22:10:20 +01:00 · 45577a1f4c
commit 45577a1f4c
parent cbb6764fb1
10 changed files with 43 additions and 44 deletions
--- 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 >
--- a/src/catch2/catch_test_case_info.cpp
+++ b/src/catch2/catch_test_case_info.cpp
@ -10,6 +10,7 @@
 #include <catch2/catch_test_spec.hpp>
 #include <catch2/interfaces/catch_interfaces_testcase.hpp>
 #include <catch2/internal/catch_string_manip.hpp>
+#include <catch2/internal/catch_case_insensitive_comparisons.hpp>

 #include <cassert>
 #include <cctype>
@ -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<TestCaseInfo>
@ -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 ) {
--- 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);
--- 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 {
--- a/src/catch2/internal/catch_list.cpp
+++ b/src/catch2/internal/catch_list.cpp
@ -13,6 +13,7 @@
 #include <catch2/interfaces/catch_interfaces_testcase.hpp>
 #include <catch2/interfaces/catch_interfaces_reporter_factory.hpp>
 #include <catch2/internal/catch_move_and_forward.hpp>
+#include <catch2/internal/catch_case_insensitive_comparisons.hpp>

 #include <catch2/internal/catch_context.hpp>
 #include <catch2/catch_config.hpp>
@ -32,12 +33,12 @@ namespace Catch {
            auto const& testSpec = config.testSpec();
            std::vector<TestCaseHandle> matchedTestCases = filterTests(getAllTestCasesSorted(config), testSpec, config);

-            std::map<StringRef, TagInfo> tagCounts;
+            std::map<StringRef, TagInfo, Detail::CaseInsensitiveLess> 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);
                }
            }
--- a/tests/SelfTest/Baselines/compact.sw.approved.txt
+++ b/tests/SelfTest/Baselines/compact.sw.approved.txt
@ -2281,7 +2281,7 @@ InternalBenchmark.tests.cpp:<line number>: passed: Timing.result == Timing.itera
 InternalBenchmark.tests.cpp:<line number>: passed: Timing.iterations >= time.count() for: 128 >= 100
 Misc.tests.cpp:<line number>: failed: false with 1 message: '3'
 Message.tests.cpp:<line number>: failed: false with 2 messages: 'hi' and 'i := 7'
-Tag.tests.cpp:<line number>: passed: tags, VectorContains("magic-tag"_catch_sr) && VectorContains("."_catch_sr) for: { ., magic-tag } ( Contains: magic-tag and Contains: . )
+Tag.tests.cpp:<line number>: passed: testcase.tags, VectorContains( Tag( "magic-tag" ) ) && VectorContains( Tag( "."_catch_sr ) ) for: { {?}, {?} } ( Contains: {?} and Contains: {?} )
 StringManip.tests.cpp:<line number>: passed: splitStringRef("", ','), Equals(std::vector<StringRef>()) for: {  } Equals: {  }
 StringManip.tests.cpp:<line number>: passed: splitStringRef("abc", ','), Equals(std::vector<StringRef>{"abc"}) for: { abc } Equals: { abc }
 StringManip.tests.cpp:<line number>: passed: splitStringRef("abc,def", ','), Equals(std::vector<StringRef>{"abc", "def"}) for: { abc, def } Equals: { abc, def }
--- a/tests/SelfTest/Baselines/console.sw.approved.txt
+++ b/tests/SelfTest/Baselines/console.sw.approved.txt
@ -16278,9 +16278,9 @@ Tag.tests.cpp:<line number>
 ...............................................................................

 Tag.tests.cpp:<line number>: 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
--- 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<StringRef>()) for: {  } Equals: {  }
 # splitString
--- a/tests/SelfTest/Baselines/xml.sw.approved.txt
+++ b/tests/SelfTest/Baselines/xml.sw.approved.txt
@ -19158,10 +19158,10 @@ loose text artifact
  <TestCase name="shortened hide tags are split apart" tags="[tags]" filename="tests/<exe-name>/IntrospectiveTests/Tag.tests.cpp" >
    <Expression success="true" type="REQUIRE_THAT" filename="tests/<exe-name>/IntrospectiveTests/Tag.tests.cpp" >
      <Original>
-        tags, VectorContains("magic-tag"_catch_sr) &amp;&amp; VectorContains("."_catch_sr)
+        testcase.tags, VectorContains( Tag( "magic-tag" ) ) &amp;&amp; VectorContains( Tag( "."_catch_sr ) )
      </Original>
      <Expanded>
-        { ., magic-tag } ( Contains: magic-tag and Contains: . )
+        { {?}, {?} } ( Contains: {?} and Contains: {?} )
      </Expanded>
    </Expression>
    <OverallResult success="true"/>
--- 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<StringRef> 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]") {