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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

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

I see. Are u willing to identify some of the exceptions with me or don't you have enough time? I would like to propose the first list here. My opinion is, that we should leave some useful macros (e.g. `Ensure` from guidelines) left for manual inspection and to disable the check for these specific macros.

> I don't think a whitelist is going to cut it here...

The whitelist could be a regex or something similar. Configuring the check to allow everything with a prefix would allow poor mans namespaces for macros. For llvm it would be `LLVM_*`, blaze could be `BLAZE_*`. These are the bigger codebases i know where macro usage was reasonable. :)

> ... -- users are not going to try to figure out every conditional compilation or attribute macro definition used in their large code base;

Conditional compilation can be done with empty macros.

  #define ALLOW_EXCEPTION
  
  #ifdef ALLOW_EXCEPTION
  // exception code
  #else
  // no exception code
  #endif

That usecase is in my eyes pretty much like an include guard an therfore acceptable, and i think not diagnosed with the check currently.

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

Common constructs that I would identify as valid:

1. Compiler specific Attributes. `#define ALWAYS_INLINE __attribute__((always_inline))`

Attributes could be identified when inspecting the tokens the macro expands to. I have no clue how to implement, but i think that should even possible with string manipulations.

2. Compiler intrinsics

These are similar to attributes and could maybe even treated the same.

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.

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.

@aaron.ballman Do you agree with the list? I would ask the guideline authors about the issue and propose exceptions to the strict rules.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41648