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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

>> OpenCV isn't clean either, here the results:
>> 
>> Filter: `ASSERT*|ALL*|CAL*|CC*|CALC*|calc*|CL*|cl*|CUDA*|CV*|cv*|EXPECT*|GTEST*|FUNCTOR*|HAVE*|ICV*|IPL*|IPP*|ipp*|__itt*|ITT*|JAS*|jas*|MESSAGE*|MAX*|OCL*|opengl*|OPENCV*|TYP*`
> 
> This one worries me a bit more because of patterns like `cl*` and `calc*` -- those seem like they're not uncommon and the pattern may silence otherwise valid diagnostics.

The macros are worriesome. Instead of using proper function overloading or similar constructs, they defined macros that replace function names + arguments. I would say, these macros were laziness, because C++ provides the proper language tools to deal with the missing overloading capabilities for C functions. I removed them because some many macros did exist.

>> - it is possible to reduce macro usage to a minimal amount, and the complex macros like AST stuff can be filtered with the regex. Furthermore, restricting all macros to a "macro namespace" is possible for sure.
> 
> I'm not certain I understand what you mean by "macro namespace". Can you clarify?

With "macro namespace" i mean a common prefix for macros, like `LLVM_UNREACHABLE`, `DEBUG_..`.

> I'm still a bit worried about using regex to filter the results. It seems like any real world project is going to require somewhere between a reasonable and an unreasonable amount of configuration to reduce the noise. Perhaps as a first pass, however, it will suffice. Hopefully we can add other heuristics to reduce the false positives as we see patterns emerge from real world usage.

It was hard to find reasonable patterns. I definitely saw the usage that is not supposed to happen, like simulating inline functions, overloading, ...

>> Things I would like to add to the check:
>> 
>> - my little filtering script is valuable for developers, that want to address the macro issue. It should be added to the docs and everyone can make something based on that. It will be linux centered.
> 
> Why does the usage need to be limited to Linux?

I created small `sed` scripts with a chain of `uniq` and `sort`, so it will be UNIX, but windows falls short i guess.

>> - enforcing ALL_CAPS, including its usage
> 
> What does "including its usage" mean?

The usage of the macro in code. That means not only definition of the macro is replaced, but all occurences. This can only be done if *all* macros are treated the same. Otherwise different TU, different transformation.

>> Code transformation has the problems of scope and potentially breaking code badly, because clang-tidy wasn't run over all of the code.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41648