mirror of
https://github.com/catchorg/Catch2.git
synced 2024-11-26 07:16:10 +01:00
Disable false positive from clang-tidy
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.
This commit is contained in:
parent
bf5c58adf6
commit
22750cde0e
@ -46,7 +46,7 @@
|
|||||||
|
|
||||||
///////////////////////////////////////////////////////////////////////////////
|
///////////////////////////////////////////////////////////////////////////////
|
||||||
#define INTERNAL_CATCH_TEST( macroName, resultDisposition, ... ) \
|
#define INTERNAL_CATCH_TEST( macroName, resultDisposition, ... ) \
|
||||||
do { \
|
do { /* NOLINT(bugprone-infinite-loop) */ \
|
||||||
/* The expression should not be evaluated, but warnings should hopefully be checked */ \
|
/* The expression should not be evaluated, but warnings should hopefully be checked */ \
|
||||||
CATCH_INTERNAL_IGNORE_BUT_WARN(__VA_ARGS__); \
|
CATCH_INTERNAL_IGNORE_BUT_WARN(__VA_ARGS__); \
|
||||||
Catch::AssertionHandler catchAssertionHandler( macroName##_catch_sr, CATCH_INTERNAL_LINEINFO, CATCH_INTERNAL_STRINGIFY(__VA_ARGS__), resultDisposition ); \
|
Catch::AssertionHandler catchAssertionHandler( macroName##_catch_sr, CATCH_INTERNAL_LINEINFO, CATCH_INTERNAL_STRINGIFY(__VA_ARGS__), resultDisposition ); \
|
||||||
|
Loading…
Reference in New Issue
Block a user