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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

> Using regex could be a reasonable way out, but isn't going to solve the problem in general because not all of the macro names are ones the user has control over. Having to whitelist dozens of macros by name means this check is simply going to be disabled by the user as being low-value.

Yes. I think with the regex its exactly implemented like the rules say. But adding sugar is still reasonable. I will run the check over llvm to get a feeling how much is still left after silence `LLVM_*` and others that are clearly scoped.

> Except that's not the only form of conditional compilation I'm worried about. See Compiler.h where we do things like `#define __has_attribute(x) 0`, which is a perfectly reasonable macro to define (and is an example of a macro name outside of the user's control, so they cannot simply prefix it for a whitelist).

Having a regex allows to add `__has_attribute` to it or a pattern like `__has*`. But how many cases for such macros are left? The goal of the guidelines is to change coding style and requiring to silence _SOME_ warnings is reasonable to me.

> 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?

I think the long term goal is removing the preprocessor, but especially compiler specifics are very unlikely to get away soon. 
They themself make exceptions: ES.33: If you must use macros, give them unique names <https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es33-if-you-must-use-macros-give-them-unique-names>

>> 2. Compiler intrinsics These are similar to attributes and could maybe even treated the same.
> 
> I'm less certain about this one -- with attributes, there's syntax that we can inspect (is there an `__attribute__` or `__declspec` keyword?), but with intrinsics there's no commonality. Or are you thinking of tablegenning the list of intrinsics?

I thought mostly of `__builtin` when proposing. Checking the MSVC docs showed that not all compilers do follow such nice naming conventions. Adding a clang and gcc list of builtins might still be manageable with basic string handling.
I feel that keeping any list of all compiler intrinsics is too much.

>> 3. Compatibility with older C++ Standards Stuff like `#define OVERRIDE override` if the code is compiled with C++11 or newer might be acceptable for stepwise modernization. This can can be considered as program text manipulation that was explicitly forbidden but does not obscure code so an exception could be made. Such macros can be detected with string comparisons.
> 
> How would you generalize this? I've seen people use macros for arbitrary keywords (`#define CONST const` and `#define false false`) as well as macros for arbitrary syntax (`#define DEFAULTED {}` or `#define DEFAULTED = default`).

Not sure about all. I think new keywords like `#define OVERRIDE override` should definitly be allowed. But things like `false` not, because it already is a keyword in C++98.

I am against allowing `#define DEFAULTED` because both are perfectly valid and we ship a check for modernizing from one to the other version. This is different to `OVERRIDE`, because it adds additional value for the programmer who might be stuck at C++98.

>> 4. Debugging/Logging stuff that contains the Line and file. These can not be modeled with current C++(?), but `LineInfo` or similar exists. I don't know what its status is. I think it is ok to require manual silencing for such macros.
> 
> In addition to `__LINE__` and `__FILE__`, I think we want to include `__DATE__`, `__TIME__`, and `__func__`.

Agreed.

> I think this list is a good start, but is likely incomplete. We'll probably discover other things to add to the list when testing the check against other real world code bases to see what patterns emerge from them.

Testing Frameworks are a common source for macro usage too. They should be coverable with the regex approach, but I don't know if there are other things hidden behind the interface that would be super annoying to silence.
Same for benchmark libraries.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41648