matching a non-constable value

[dvirtz]

Description
I'm trying to match a range-v3 view which cannot be const as it mutates its state during iteration. The problem is that matchers accept their input by const reference only.

Is it possible to add additional overload which will accept the parameter by value? Or do you have any other suggestion?

Additional context
Version 2.6.0

[horenmar]

This is one of the cases where const_cast is well-defined and useful, so my suggestion would be to use it for now. Other short-term solution would be using a ref-wrapper I guess.

However, you should be really careful with things that are mutated during matching, because they can easily break if you try to chain multiple matchers in a single assertion.

As for the future, we will see what we can do about this -- there already is a problem with using const& in assertions, because it does not play nice with type traits and other values without linkage -- but I don't know of a good solution to the problem.

[dvirtz]

Thanks. A reference_wrapper is a nice idea.
I started working on a PR which replaces virtual functions with CRTP and the matcher simply forwarding its input. Would you consider accepting that?

It already compiles, I'm just working on making sure the output stays the same from the builtin matchers.

[horenmar]

Hard to say from that description alone, you might want to open up a PR that highlights the intended changes so you don't waste time if there are problems.

---

Note that the Matchers interface is subject to the usual backward compatibility guarantees, so it would have to go into v3.0 either way.

[dvirtz]

PR #1554 opened. Is there a time estimation for 3.0?

[dvirtz]

Now that v3.0 is a thing, I've rebased the PR on dev-v3. Can you please review it now?

[horenmar]

Yeah, sorry for not returning to you sooner on the issue.

I had some misgivings about the fact that your proposed PR is a breaking change to all already existing matchers. Meanwhile, back in October @melak47 offered an alternative design seen in #1843, that allows keeping the old matchers while forward extending them with templated ones.

Would the design in #1843 work for what you want to do?

[dvirtz]

It would work but the performance will be much worse because of virtual functions and type erasure in #1843 .
I ran some benchmarks to illustrate this:
Using EqualsRangeMatcher defined in https://github.com/melak47/Catch2/blob/43b123f83f69c7a75918c80954a106e428d27a7d/tests/SelfTest/UsageTests/Matchers.tests.cpp#L561, the following test:

TEST_CASE("Combining templated matchers", "[matchers][templated]") {
    std::array<int, 3> container{{ 1,2,3 }};

    std::array<int, 3> a{{ 1,2,3 }};
    std::vector<int> b{ 0,1,2 };
    std::list<int> c{ 4,5,6 };

    for(int i = 0; i < 10000000; ++i) {
        REQUIRE_THAT(container, EqualsRange(a) || EqualsRange(b) || EqualsRange(c));
    }
}

using @melak47 solution takes 3.57s and with mine 0.27s. The test

TEST_CASE("Combining templated and concrete matchers", "[matchers][templated]") {
    const std::string str = "foobar";
    const std::array<char, 6> arr{{ 'f', 'o', 'o', 'b', 'a', 'r' }};
    const std::array<char, 6> bad_arr{{ 'o', 'o', 'f', 'b', 'a', 'r' }};

    using Catch::Matchers::StartsWith;
    using Catch::Matchers::EndsWith;
        
    for(int i = 0; i < 10000000; ++i) {
        REQUIRE_THAT(str, StartsWith("foo") && EqualsRange(arr) && EndsWith("bar"));
        REQUIRE_THAT(str, StartsWith("foo") && !EqualsRange(bad_arr) && EndsWith("bar"));
    }
}

takes 8.54s using @melak47 's solution and 2.25s with mine.
This is with GCC 9.2 on a MacOS Catalina machine with 2.8 Quad core Intel I7.

I'll try to change my PR to be non-breaking as I understand it's a big issue.

[horenmar]

Can you also try creating the composed matcher outside the loop? Right now it uses vector (which implies allocations) to avoid compiling differently sized arrays.

[dvirtz]

I ran it and with #1554 the run times are 0.75s for both benchmarks.
For #1843, the first one runs for 1.01s and the second one crashes.

[melak47]

The overhead in my PR does indeed seem to come from using vectors.
I tried using arrays instead, which did not have as much of an impact on compile times as I expected (it actually seems to compile faster...).

The results for your heavier test case on my machine:
crtp: 2.51s, virtuals & vectors: 4.51s, virtuals & arrays: 1.48s

I imagine the remaining difference comes from you actually storing matchers in a tuple, while I only store pointers to the matchers (like the existing MatchAllOf in master).

[dvirtz]

It's a problem to store a pointer if the matcher is temporary. I stored references at first but it had the same problem.

…

On Mon, Feb 10, 2020, 02:20 melak47 ***@***.***> wrote: The overhead in my PR does indeed seem to come from using vectors. I tried using arrays instead, which did not have as much of an impact on compile times as I expected (it actually seems to compile faster...). The results for your heavier test case on my machine: crtp: 2.51s, virtuals & vectors: 4.51s, virtuals & arrays: 1.48s I imagine the remaining difference comes from you actually storing matchers in a tuple, while I only store pointers to the matchers (like the existing MatchAllOf in master). — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1553?email_source=notifications&email_token=AB2ORDIE6QPZ6C7NVB52IQ3RCCMWJA5CNFSM4G2SXRKKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELG4Z3A#issuecomment-583912684>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB2ORDNPMSSUKWRVF3NUQQDRCCMWJANCNFSM4G2SXRKA> .

[dvirtz]

Well, storing by value isn't working for classes deriving from MatcherBase so I went back to storing references for now as combining temporary matchers is not supported even today.

[horenmar]

Storing around just references (well, pointers) in the composed matchers is intentional and I intend to keep it around.

@melak47 Can you try to benchmark the differences in compilation times between using std:array and std::vector for storage?

[melak47]

measured on windows:

clang, std::vector: 3.56s
clang, std::array: 2.58s

msvc, std::vector: 1.74s
msvc, std::array: 1.65s

gcc, std::vector: 3.28s
gcc, std::array: 3.87s

[melak47]

Actually, those measurements were release builds, so maybe not entirely fair comparing compile times. This time just debug builds:

compiler	variant	compile time	run time
gcc	crtp	3.64s	1.41s
gcc	vector	1.80s	2.39s
gcc	array	1.77s	1.41s
clang	crtp	3.40s	13.02s
clang	vector	1.15s	17.00s
clang	array	1.25s	13.25s
msvc	crtp	2.07s	28.43s
msvc	vector	1.02s	34.55s
msvc	array	1.06s	28.81s

These are the sources used for this test, run time was measured for test case "b" (although they are all over the place, maybe not fair to compare debug builds' run times): https://gist.github.com/melak47/8d8429c102fda933ce98dca31cb400fc

[horenmar]

So, to sum it up, there are currently 3 options for the new Matchers:

• dvirtz's CRTP based
• melak's using std::array for backing storage
• melak's using std::vector for backing storage

As far as I can see, they can be summed as

CRTPs

• fastest at runtime
• slowest to compile
• breaks old matchers

vectors

• (usually) fastest to compile
• by far slowest at runtime
• causes bunch of extra allocs at runtime (aka the reason it is so slow)
• compatible with old matchers

arrays

• slightly slower to compile than vectors
• slightly slower at runtime than CRTP
• compatible with old matchers

Given that the vector backed approach is only slightly faster to compile
than array backed approach, and is the slowest at runtime by a significant
margin, it can be safely dropped from further consideration.

This leaves arrays and CRTPs, and given that the runtime advantage of CRTPs
is small, potential compile time advantage of arrays significant, and
that the array-oriented approach is compatible with the old matchers,
and that the array backed approach is compatible with old matchers,
I am very much inclined to merge it.

@dvirtz I am sorry your work will end up being thrown away, but I think
you also helped to improve the final version of new matchers significantly,
because without your input the array version would not have been tested.

[dvirtz]

The CRTP PR doesn't actually breaks compatibility anymore. I agree that compile time is not great though.