Clang Tidy: Align and setup first set of rules

As discussed in the sync meeting on 10.11.2022, we need to align on some Clang Tidy checks for our repository. Thus, please provide suggestions and examples here in the ticket. After collecting all inputs, we will proceed by consolidating and come up with a first draft to start with.

@rparisha2 @jdobberstein @mbauerm6f @arauschert Looking forward to your suggestions.

added Community Contribution label

assigned to @rparisha2

@adascri Have you already discussed, if you want the ClangTidy config to have minimal impact (at least at first) or if you want to take the opportunity to modernize and homogenize the codebase? Especially the cppcoreguidelines-* and modernize-* categories of checks would have quite an impact.

Here's a proposal with minimal impact: https://gitlab.eclipse.org/mstump/scenario_api/-/blob/b49fbdb574db8bf3928b4315c3312a2979e6e80f/.clang-tidy

The exceptions (lines starting with "-") could be taken back one after the other, and the codebase would be adapted accordingly.

@mstump We have not made a final decision on this yet. Instead, we will have a look at all the suggestions and come up with a proposal. My personal opinion is that we need to find some compromise between not having very few rules only and having to refactor quite a lot. Thus, it depends on the effort for adapting the codebase which we will see when applying the rules.

Thank you for your proposal! The cppcoreguidelines also came up in our meeting on Thursday.

Many of the rules in question offer automatic fixes (see Clang-Tidy Checks), so even if we choose to apply them all, the time and effort would be manageable. My suggestion would be to apply all the rules that do not break the API, and discuss those that do one by one.

mentioned in merge request !74 (closed)

Thank you @mstump for your great contribution!

We've checked the set with respect to mantle and the openscenario engine and would like to see the following changes (discussed).

REMOVED:

1. llvm-header-guard - or do you know how to restrict that to "public" headers only + we use the non-standard pragma once a lot
2. llvm-qualified-auto - no extra pointer please when using auto.
3. readability-qualified-auto(same as 2)

ADDED:

1. modernize-return-braced-init-list
2. modernize-use-nodiscard
3. cppcoreguidelines-virtual-class-destructor
4. bugprone-easily-swappable-parameters
5. cppcoreguidelines-pro-type-static-cast-downcast

UNSURE:

• misc-non-private-member-variables-in-classes - right now, the engine uses protected members within base classes

TESTING:

• google-readability-avoid-underscore-in-googletest-name
• how do we deal with that anyhow - an extra file - any experience with it?

see also

• Comments
• Clang-tidy_check

@rparisha2 Thank you for the detailed analysis. I will work through the list and adjust the configs accordingly.

I would however strongly advise against disabling *-qualified-auto. I remember a bug in some internal code, where the & was missing on a variable returned from a function. The variable was copied (default behavior) instead of referenced. The supposed reference was then assigned to, which of course did not have the intended effect.

Hence, from my experience, the reasoning behind https://llvm.org/docs/CodingStandards.html#beware-unnecessary-copies-with-auto is sound.

Re. TESTING, configs can include other configs and override the list of checks. We do the same in our internal testing code.

created branch 20-clang-tidy-align-and-setup-first-set-of-rules to address this issue

mentioned in merge request !78 (merged)

mentioned in commit openscenario1_engine@0bb372d3

mentioned in merge request openscenario1_engine!96 (closed)

mentioned in commit 18b7a9be

closed with merge request !78 (merged)