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

Sat Jan 13 08:20:31 PST 2018

aaron.ballman added a comment.

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

> > 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 can try to help as I have time.

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

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.

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

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

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

Agreed.

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

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

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

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

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.

================
Comment at: docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst:8
+constructs exist for the task.
+The relevant sections in the C++ Coreguidelines are 
+`Enum.1 <https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#enum1-prefer-enumerations-over-macros>`_,
----------------
Coreguidelines -> Core Guidelines

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41648