Evaluate argument of (DYNAMIC_)SECTION, TEST_CASE in static-analysis mode

[mjerabek]

Description

As the title says, it's fairly straightforward.

And also add configuration for clang-tidy, run it in CI, and fix/suppress the resulting warnings.

GitHub Issues

Closes #2816

[mjerabek]

Not sure if/where should I add tests for this.

[codecov]

Codecov Report

Attention: Patch coverage is 96.87500% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 91.16%. Comparing base (c3fd4eb) to head (fcc7c07).

Additional details and impacted files

@@            Coverage Diff             @@
##            devel    #2817      +/-   ##
==========================================
- Coverage   91.20%   91.16%   -0.04%     
==========================================
  Files         197      197              
  Lines        8374     8381       +7     
==========================================
+ Hits         7637     7640       +3     
- Misses        737      741       +4     

[horenmar]

I am reasonably sure that this breaks SECTION macros in some cases, so a test would be nice 😛 .

This is a valid SECTION start: SECTION("short", "long description with many many words"), and unless I am very mistaken, your change would break it.

[mjerabek]

Huh, interesting. I had no idea it can be used like that 😄

Any tips where the tests for this should be?

[mjerabek]

• fixed the implementation (yes, the tests uncovered other bugs)
• added basic clang-tidy run to CI

Some limitations:

• Some files are checked multiple times, because they are present in compile_commands.json multiple times with different compile options. Also I'm quite certain that run-clang-tidy doesn't handle this well and we run it multiple times with the same configuration.
• run-clang-tidy doesn't stop on error, so it always runs in full. If we used cmake's clang-tidy integration, we could rely on ninja. But cmake doesn't support running clang-tidy without also running the compiler, so it would either take longer, or we would have to fake a "null compiler" (I've done that in work, it works well, but it's not very nice). Or we could put together a short python script that generates a ninja build file and run that (I would probably slightly prefer that to the current solution, as we could also better deal with duplicate files in compile_commands.json).
• Only the default checks are enabled at the moment. We should probably explicitly define what we want to check and what we want to consider an error.
• If there are errors, the job fails (good). But we ignore any warnings. 🤷

Also, I might add some clang-tidy-specific tests later to test that clang-tidy understands that REQUIRE(expr) implies expr (and also implement it, because now it most probably cannot understand that). But that's for a follow-up PR.

[horenmar]

Suggested change

	build_description: Extras + Examples
	build_description: clang-tidy

instead?

[mjerabek]

"clang-tidy" is already included in the job description. I've replaced this by "all", but it's not really completely all. I guess it's hard to put variant-specific description when there is just one variant.

[horenmar]

Right, then "extras + examples" is fine.

[horenmar]

run-clang-tidy doesn't stop on error, so it always runs in full. If we used cmake's clang-tidy integration, we could rely on ninja. But cmake doesn't support running clang-tidy without also running the compiler, so it would either take longer, or we would have to fake a "null compiler" (I've done that in work, it works well, but it's not very nice). Or we could put together a short python script that generates a ninja build file and run that (I would probably slightly prefer that to the current solution, as we could also better deal with duplicate files in compile_commands.json).

That's not great. Maybe we could merge the clang-tidy check into an already existing build? This would create a build with outsized build time, but we can't build all the builds in parallel either way, so as long as it gets scheduled reasonably early it shouldn't have much impact.

Some files are checked multiple times, because they are present in compile_commands.json multiple times with different compile options. Also I'm quite certain that run-clang-tidy doesn't handle this well and we run it multiple times with the same configuration.

Yes, they are indeed compiled multiple times in the full test build. Some tests require building against custom configuration of the implementation binary, so they have to rebuild it. You can find them quickly by looking for targets that link against Catch2_buildall_interface.

Only the default checks are enabled at the moment. We should probably explicitly define what we want to check and what we want to consider an error.

Definitely. The checks in clang-tidy can be a mess, with overlapping checks with different implementation, this came up in the original discussion for the static-analysis mode. Having a reasonable set that we can point to as being supported would be nice.

Also, I might add some clang-tidy-specific tests later to test that clang-tidy understands that REQUIRE(expr) implies expr (and also implement it, because now it most probably cannot understand that).

It almost definitely cannot understand it. The reasoning there is only slightly simpler than figuring out SECTIONs, and the analyzer helper mode has to help with those a lot. However, from cursory check, there is a relatively reasonable way of telling clang-tidy about custom assertions. It would require making the REQUIRE macro also change its definition based on the defines though.

[mjerabek]

In the end, I have rewritten the CI job to use cmake to run clang-tidy, with a fake compiler (turned out it's not messy when you're not adding PCHs and ccache into the mix, which I now realize are both useless for just clang-tidy run; oh well, I'm putting that on the endless list of pending CI refactorings).

The clang-tidy checks are now explicitly configured. The rationale is following: allow-list whole categories, deny-list particular checks from the categories. This way, when a new clang version adds new checks, they are easily discovered (if they print a warning) and may be acted upon - either fix the findings or disable the check if it is deemed not useful. YMMV

Werror is enabled for some concrete checks.

Unfortunately, we cannot enable bugprone-use-after-move. First, it cannot be made to understand our custom unique_ptr (llvm/llvm-project#36901). Second, it doesn't understand that if (i == 1) and if (i == 2) are exclusive, so it doesn't work with the SECTION hinting.

We also have to disable bugprone-macro-repeated-side-effects because if wrongly triggers for CATCH_MOVE and CATCH_FORWARD, although the second expansion of the macro argument is in unevaluated context (no issue for that yet AFAICT).

[mjerabek]

So, I have split each check to its separate commit as you wanted. It's up to you now to select which ones do you want to keep and which ones you want to suppress instead.

[horenmar]

What's supposed to be duplicated here?

[mjerabek]

The else branch waaay down.

[horenmar]

ugh, that's dumb

[mjerabek]

Very 😄