Clang-tidy is smart enough to understand that the conditional is never
updated in the loop body. It will let you get away with it if it can
prove that the conditional is always false, but that is not always
possible.
Here is an example where it's not able to prove it, and thus gives a
false positive. This is a minimal reproduction of an actual case I hit
in production, where `function` is picking the function based on some
`constexpr` logic related to which type argument is currently being
tested.
```
int f();
TEMPLATE_TEST_CASE("reproduction", "", int) {
const auto function = []() {
return f;
}();
const int error = function();
REQUIRE(error == 0); // clang-tidy complains: bugprone-infinite-loop
}
```
I did not choose to add this test to the test suite, since we're not
running `clang-tidy` in CI afaik. To run it manually, simply add the
snippet above somewhere and run clang-tidy with
`--checks=bugprone-infinite-loop`. Or see an example at
https://godbolt.org/z/4v8b8WexP.
The reason we get the infinite loop warning in the first place is the
conditional at the end of this `do`-loop. Ideally, this conditional
would just be `while(false)`, but the actual content of the
`REQUIRE`-statement has been included here too in order to not loose
warnings from signed/unsigned comparisons. In short, if you do
`REQUIRE(i < j)`, where `i` is a negative signed integer and `j` is an
unsigned integer, you're supposed to get a warning from
`-Wsign-compare`. Due to the decomposition in Catch2, you lose this
warning, which is why the content of the `REQUIRE` statement has been
added to the conditional to force the compiler to evaluate the actual
comparison as well.
This was discussed on Discord today, and an alternative approach (which
I don't have time to implement) would be to in the decomposition replace
the comparison operators with `cmp_less` and friends. These are C++20
though, and would have to be implemented manually. I am also not sure
it's a good idea to "magically" change the semantics of `<` when it's
used inside a `REQUIRE` macro.
Another alternative approach would be to trigger this warning in a
different way, by including the content of the `REQUIRE` macro in a
different way which doesn't affect the for loop. But I don't have enough
of an overview here to know where would be a good place and how to test
that I didn't break anything.
We used to use whatever precision we ended up having from C++'s
stdlib. However, some relatively popular tools, like Jenkins,
use Maven SureFire XML schema to validate JUnit test reports, and
Maven SureFire schema requires the duration to have at most 3
decimal places.
For compatibility, the JUnit reporter will now respect this
limitation.
Closes#2221
There is no good reason to provide a "add empty line" primitive
for writing XML documents, and the fact that it remains unused
after all the time it was provided only confirms this further.
This decreases code size and improves performance of passing around
`unique_ptr` instances by value somewhat. It virtually guarantees
problems when combining code compiled with Clang and GCC, but that
was never supported anyway.
In b7b346c3e5 this conditional was simplified to just
`while( false)` rather than the current one. Then in 3a33315ff8, that
change was reverted. I found it hard to understand this
complicated conditional, but after some digging in history I found this
comment which used to be here. It was removed in b7b346c3e5, but not
restored together with the revert in 3a33315ff8. Let's revive it.
It used to be part of the experimental benchmarking support, but
since that was replaced with proper benchmarking support with its
own timer facilities, it is now a dead code and useless.
The problem with the old name was that it collided with the
range matcher `Contains`, and it was not really possible to
disambiguate them just with argument types.
Closes#2131
This change also changes it so that test case macros using a
class name can have same name **and** tags as long as the
used class name differs.
Closes#1915Closes#1999
They now take `StringRef` as the argument, and are virtual only
in the basic interface.
Also cleaned out the various reporters and their overrides
of these members which were often empty or delegating up.
This means that e.g. for `TEST_CASE` with two sibling `SECTION`s
the event will fire twice, because the `TEST_CASE` will be entered
twice.
Closes#2107 (the event mentioned there already exists, but this
is its counterpart that we also want to provide to users)
This means that it can no longer be safely made ahead of time,
but nothing in our existing code used it like that. Normally it
is constructed and used in the same expression, which is now
more efficient.
Using the `CATCH_MOVE` and `CATCH_FORWARD` macros instead of the
`std::move` and `std::forward<T>` utility functions can improve
compilation times and debug build's performance, and thus will
be preferred going forward.
With these changes, all these benchmarks
```cpp
BENCHMARK("Empty benchmark") {};
BENCHMARK("Throwing benchmark") {
throw "just a plain literal, bleh";
};
BENCHMARK("Asserting benchmark") {
REQUIRE(1 == 2);
};
BENCHMARK("FAIL'd benchmark") {
FAIL("This benchmark only fails, nothing else");
};
```
report the respective failure and mark the outer `TEST_CASE` as
failed. Previously, the first two would not fail the `TEST_CASE`,
and the latter two would break xml reporter's formatting, because
`benchmarkFailed`, `benchmarkEnded` etc would not be be called
properly in failure cases.
This is a simplification of the fix proposed in #2152, with the
critical function split out so that it can be tested directly,
without having to go through the ULP matcher.
Closes#2152
In v2 it was placed in a very central header due to the way it was
stitched together. Now that we don't do that, we can move it to the
proper place, removing the potential for confusion given that the
original header was split apart and renamed.
- NVHPC's implementation of `__builtin_constant_p` has a bug which
results in calls to the immediately evaluated lambda expressions to be
reported as unevaluated lambdas.
https://developer.nvidia.com/nvidia_bug/3321845.
- Hence, we disable CATCH_INTERNAL_IGNORE_BUT_WARN for NVHPC Compilers
This let's us avoid running `strlen` at runtime to convert the
plain string literals to `StringRef`s, by guaranteeing that we
instead have the size available after compilation.
In optimized builds the performance improvement should be even
greater, as the `StringRef` UDL and the related constructor
are both `constexpr`, and thus can be baked completely during
compilation.
Previously, string literals and `std::string`s would match the
template variant, which would serialize them into a stream and then
call the `StringRef` overload for resulting string. This caused
bunch of codebloat and unnecessary pessimization for common usage.
This introduces a potential lifetime risk when using the API, but
the intended way to use the `XmlEncode` class is to use it directly,
e.g. `out << XmlEncode(some-text-argument)`, not to store it around.
The benefit is that we avoid allocations for strings that do not fit
into SSO for given platform.
In some places the `std::flush` was not added, as it was sufficiently
obvious that the flush semantics are not intended. There are likely
other places where the flush semantics aren't intended, but that
is a cleanup for later.
More specifically, made the actual implementation of string-like
type handling take argument as `Catch::StringRef`, instead of
taking `std::string const&`.
This means that string-like types that are not `std::string` no
longer need to pay for an extra construction of `std::string`
(including the potential allocation), before they can be stringified.
The actual string stringification routine is now also better about
reserving sufficient space.
Apart from being clearer, it also improves the overall codesize
of the implementation library, and should improve the performance
as well, by removing one level of indirection.
Because new glibc has changed `MINSIGSTKSZ` to be a syscall instead
of being constant, the signal posix handling needed changes, as it
used the value in constexpr context, for deciding size of an array.
It would be simple to fix it by having the handler determine the
signal handling stack size and allocate the memory every time the
handler is being installed, but that would add another allocation
and a syscall every time a test case is entered.
Instead, I split apart the idea of preparing fatal error handlers,
and engaging them, so that the memory can be allocated only once
and still be guarded by RAII.
Also turns out that Catch2's use of `MINSIGSTKSZ` was wrong, and
we should've been using `SIGSTKSZ` the whole time, which we use now.
Closes#2178
* [Issue 2154] Correct error when building with IBM's latest XLC compiler with xlclang++ front-end.
On AIX, the XLC 16.1.0.1 compiler considers the call to `std::abs` ambigious, so it needs help with a static_cast to the type of the template argument.
Co-authored-by: Martin Hořeňovský <martin.horenovsky@gmail.com>
Also generalized the implementations to write to the provided
output stream, which will be required for the follow up changes,
where the listings should happen to the location user asked for
by specifying the `-o` flag.
Previously, every base derived from the IStreamingReporter had
its own `IConfig const* m_config` member, so this just centralizes
the handling thereof.
Part of #2061
The old name was a legacy of v2 era, where all headers were stitched
into one. With v3 using separate headers, it is better when they have
proper name.
Previous splitting of catch_common.hpp left it containing only one
actual thing, which is the `SourceLineInfo` type. Given that, there
is no reason to keep the old name.
Also found out that it was included in some places for no reason
(primarily Matchers).
