Comparison expression creation fix

[AlCash07]

Description

Why.
This PR fixes the issue that user-defined operators can be a better match than the ones used to create Catch2 expression (defined in Decomposer and ExprLhs).

What.

• Overload with forwarding reference is added for all the comparison operators used in expressions.
• This overload doesn't compile for bit field non-const reference, so another overload that takes all arithmetic types by value is added. It replaces bool overload where it existed before.
• Operators are now defined as hidden friends to fix compilation on GCC.

GitHub Issues

Closes #2121

Note

I'm sorry for not fully reading the contribution guidelines. I just have a problem and propose a solution. Feel free to edit my code.

[codecov]

Codecov Report

Merging #2128 (89d93ea) into devel (65c9a1d) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head 89d93ea differs from pull request most recent head 1f6c66b. Consider uploading reports for the commit 1f6c66b to get more accurate results

@@            Coverage Diff             @@
##            devel    #2128      +/-   ##
==========================================
- Coverage   90.30%   90.28%   -0.02%     
==========================================
  Files         146      146              
  Lines        7141     7127      -14     
==========================================
- Hits         6448     6434      -14     
  Misses        693      693              

[AlCash07]

I compared compilation time with the current master by running these commands:

rm -rf debug-build
cmake -Bdebug-build -H. -DCMAKE_BUILD_TYPE=Debug -DCATCH_BUILD_EXAMPLES=ON -DCATCH_BUILD_EXTRA_TESTS=ON -DCATCH_DEVELOPMENT_BUILD=ON
cmake --build debug-build --target SelfTest

Compilation time for separate targets I got after one run:

Before the change:
Catch2: 33 sec
SelfTest: 28 sec

After the change:
Catch2: 32 sec
SelfTest: 30 sec

Regression in total is around 1.5%, but one run is obviously not statistically significant.
The conclusion I'd make is that there is no significant regression.

[AlCash07]

Compared compilation time again using this command

hyperfine --prepare 'rm -rf debug-build | cmake -Bdebug-build -H. -DCMAKE_BUILD_TYPE=Debug -DCATCH_BUILD_EXAMPLES=ON -DCATCH_BUILD_EXTRA_TESTS=ON -DCATCH_DEVELOPMENT_BUILD=ON' 'cmake --build debug-build --target SelfTest'

Before the change:

Command	Mean [s]	Min [s]	Max [s]	Relative
cmake --build debug-build --target SelfTest	63.719 ± 0.803	62.096	64.572	1.00

After the change:

Command	Mean [s]	Min [s]	Max [s]	Relative
cmake --build debug-build --target SelfTest	64.315 ± 0.694	62.726	64.832	1.00

[horenmar]

That's not good measurement: you explicitly include the time needed to build the static library proper, which is unaffected. However, I've had time to collect some measurements on my own:

This PR:

$ hyperfine -w 2 --prepare 'find ../tests/SelfTest/ -type f -name "*.cpp" -exec touch {} +' 'ninja -j 1'
Benchmark #1: ninja -j 1
  Time (mean ± σ):     19.532 s ±  0.463 s    [User: 18.527 s, System: 0.987 s]
  Range (min … max):   18.215 s … 19.693 s    10 runs

current devel

$ hyperfine -w 2 --prepare 'find ../tests/SelfTest/ -type f -name "*.cpp" -exec touch {} +' 'ninja -j 1'
Benchmark #1: ninja -j 1
  Time (mean ± σ):     18.914 s ±  0.720 s    [User: 17.922 s, System: 0.975 s]
  Range (min … max):   17.845 s … 19.694 s    10 runs

So that's difference of roughly 600ms for the median, which is about 3% slowdown over the current status. Not great, not terrible...

I will also try what happens if, instead of doing SFINAE for taking things by-value for arithmetic types, we return to the old functionality of only taking bools by value, which should require less work from the compiler to accomplish.

---

e: ok, plain overload won't work due to bitfields, and I assume that's why you went the is_arithmetic route instead 😃

[AlCash07]

I just realized that the new test is added here, and it would be fair to compare compilation times without this test.

[AlCash07]

@horenmar I've learned about the "hidden friends" idiom, so I moved operators inside the class. I repeated your measurements, on my laptop they produce:

Current devel:

Benchmark #1: ninja -j 1
  Time (mean ± σ):     29.597 s ±  0.284 s    [User: 26.704 s, System: 2.309 s]
  Range (min … max):   29.071 s … 29.917 s    10 runs

This PR before the update:

Benchmark #1: ninja -j 1
  Time (mean ± σ):     30.549 s ±  0.560 s    [User: 27.571 s, System: 2.375 s]
  Range (min … max):   29.827 s … 31.406 s    10 runs

This PR now:

Benchmark #1: ninja -j 1
  Time (mean ± σ):     29.581 s ±  0.491 s    [User: 26.668 s, System: 2.313 s]
  Range (min … max):   29.129 s … 30.670 s    10 runs

[horenmar]

Thanks, the new compile time performance looks pretty good.

However, I think the change in inferred types is wrong for two reasons:

• it stops the assertions from being used with non-movable-non-copyable types. (I thought they were represented in the tests already, but turns out they are only tested in matchers, not plain assertions.)
• It causes needless runtime overhead in types that can be moved/copied.

As to the concern for dangling refs, the critical part of the code is this

catchAssertionHandler.handleExpr( Catch::Decomposer() <= __VA_ARGS__ );

any temporary created as part of __VA_ARGS__ should be alive until handleExpr returns, at which point Catch2 no longer keeps around the references to them. This means that it is fine to keep the inferred types as references.

[horenmar]

Apart from the main problem, 2 small nitpicks.