[PATCH] D41648: [clang-tidy] implement cppcoreguidelines macro rules

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D41648#965437, @JonasToth wrote:

> In https://reviews.llvm.org/D41648#965432, @aaron.ballman wrote:
>
> > I think this check is going to be extraordinarily chatty. For instance, macros are often used to hide compiler details in signatures, such as use of attributes. This construct cannot be replaced with anything else because the macro isn't defining an object or value. Another example where this will be bad is for conditional processing where the macro is later used in a `#if`, `#elif`, `#ifdef`, or `#ifndef` preprocessor conditional, as this also cannot be replaced with a `constexpr` variable. Without handling things like this, I don't see how this rule can provide utility to real world code. Do you have ideas for how to handle these kind of situations?
>
>
> The check will only warn in the definition of the macro not the expansion.


I understand. It's the definitions I am concerned about.

> The guidelines are really strict with macros and explicitly state that program maniupulation is a bad thing.

Yes, and I'm saying that the guidelines aren't useful for real code bases because they restrict more than is reasonable. So while I think the check is largely implementing what the guidelines recommend, I think that some of these scenarios should be brought back to the guideline authors to weigh in on before we expose the check to users.

> Having a macro without value, like header guards or compile time flag style things are not reported with this check.
> 
> Other (valid) use cases require a NOLINT with this check. But i do not know how to figure out automatically what a macro is meant to do. I can introduce a whitelist for allowed macros.
>  Furthermore i could make each use case a configurable option. E.g. forbidding constant definitions i still a good thing (imho).

I don't think a whitelist is going to cut it here -- users are not going to try to figure out every conditional compilation or attribute macro definition used in their large code base; they'll simply disable the check as not being overly useful. For example, look at Compiler.h in LLVM and try to see how you would rewrite that code to perform the same purposes without triggering diagnostics from this check. That sort of file is common to almost every production code base I've ever come across.

One way to make this less chatty would be to check whether the macro replacement list is defining a source construct that cannot be replaced by constexpr variables or inline functions (such as LLVM_ATTRIBUTE_ALWAYS_INLINE). If we had whole-program analysis, I think we could do something further like scan the uses of the macros to determine whether they're used for conditional compilation (such as LLVM_ENABLE_EXCEPTIONS). However, I have no idea how we would handle cases like LLVM_LIKELY and LLVM_UNLIKELY, but expecting users to NOLINT 500+ lines of common macro code doesn't seem like a good first approach. I'd be curious to know what the C++ Core Guidelines folks think about those use cases and how they would recommend rewriting the code to adhere to their guidelines. Do they really expect users to use *no* macros in C++ code outside of header include guards and noop definitions even when asked about reasonable use cases like cross-compiler attributes or builtins? I mean, many of these things cannot even use the attribute the C++ Core Guidelines require for silencing such diagnostics -- how do they want to use gsl::suppress to silence a diagnostic on a macro definition?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41648