Skip to content

Commit

Permalink
Disable false positive from clang-tidy
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
knatten authored and horenmar committed Oct 21, 2021
1 parent bf5c58a commit 22750cd
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion src/catch2/internal/catch_test_macro_impl.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@

///////////////////////////////////////////////////////////////////////////////
#define INTERNAL_CATCH_TEST( macroName, resultDisposition, ... ) \
do { \
do { /* NOLINT(bugprone-infinite-loop) */ \
/* The expression should not be evaluated, but warnings should hopefully be checked */ \
CATCH_INTERNAL_IGNORE_BUT_WARN(__VA_ARGS__); \
Catch::AssertionHandler catchAssertionHandler( macroName##_catch_sr, CATCH_INTERNAL_LINEINFO, CATCH_INTERNAL_STRINGIFY(__VA_ARGS__), resultDisposition ); \
Expand Down

0 comments on commit 22750cd

Please sign in to comment.