Though I did find that it inserted more than one NOLINT comment for a templated function, probably due to multiple warnings being issued for the same violation.

I just tried this out on a somewhat larger scale - I built all of CMake with this flag enabled, and it found a number of clang-tidy violations and added NOLINT comments to silence them. Worked like a charm.

Added virtual destructor to HeaderGuardStyle.

CI on all seven commits in this stack looks good.

Removed comma from documentation.

Updated documentation.

Updated with review feedback.

@aaron.ballman @njames93 could you guys please take another look at this?

Any chance we could get the rest of LLVM upgraded to 3.20.0 as well? I'd like to get Ninja Multi-Config working.

Fixed formatting.

Rebased onto D143099.

Added tests and fixed assertion error in release build.

Fixed crash when warning doesn't have associated location.

In D142673#4086018, @carlosgalvezp wrote:

Note: I do not have possibility to add code comments - is there some too strict permissions set for this patch? It has not happened to other patches.

Closed in favor of D142673.

Refactored to use HeaderGuardStyle.

Rebased onto main for D141000.

Rebased onto main for D141000.

Rebased onto main for D141000.

Rebased onto main for D141000.

Rebased onto main for D141000.

Printed Introducer parameter in Pragma methods.

Added Introducer parameter to Pragma callbacks.

In D142123#4071099, @njames93 wrote:

Can you just elaborate on what the registry is used for? Is the plan to support potentially dynamically loading HeaderGuardStyle classes.

Basically, anything that extends HeaderGuardCheck (or HeaderGuardBase from D142121) is really just a fancy configuration option, rather than a separate check, and should be treated as such. My proposal is this:

In D142123#4066706, @KyleFromKitware wrote:

The only way to avoid this is if we overhaul the existing HeaderGuardCheck and make its naming convention configurable by a configuration option as opposed to having to extend the C++ class to calculate the name.

In D142123#4066676, @aaron.ballman wrote:

It doesn't have to -- for example, we can name the guard based on the path to the header. e.g., foo/include/bar/baz.h could use FOO_INCLUDE_BAR_BAZ_H as the header guard.

In terms of a "generic check", there is already the HeaderGuardCheck, but that has to be extended to be specific to the project's naming convention. And I don't think there can be a generic check that converts #pragma once to a header guard - when it adds the header guard, what does it name it? It has to know the project's naming convention. If we want a check that replaces #pragma once with a header guard, our best bet would be modifying HeaderGuardCheck to do this, since it already does most of the needed work. However, such functionality is orthogonal to my proposal here.

In D142123#4066460, @aaron.ballman wrote:

We're trying to figure out what the correct approach is (fwiw, I'd be opposed to what's proposed as-is but would be fine with a tweaked proposal).

Yes, this is for D142123. However, even if that doesn't land, I'd still like to land this. We use the code from D142123 in CMake's CI system, and getting it to work without this change resulted in a massive amount of code duplication. This refactoring would allow us to eliminate such duplication on our end.

In D142123#4066351, @njames93 wrote:

and peoples eagerness to blindly enable all checks (or all checks from a module)

We have this check in a custom clang-tidy module that runs on CMake's CI system. Other people have also sought out such a check, and I figured it made sense to upstream it instead of keeping it stuck in our CI. Even though it's not standard, other people might still want to use it. Perhaps there's a better category for it than modernize?

Added documentation.

WIP - currently updating the tests for pp-trace and in the process debugging an issue with the location of the #endif hash.