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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

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

> I checked several codebases and implemented a little helper script to see which kind of macros exist. Then I added some filter regex as configuration and tried again, here are the results:
>
> For https://github.com/nlohmann/json which is a very high quality codebase, the results were as expected: very clean, and good macro usage: (except a `#define private public` for tests)
>
> Filter: `JSON*|NLOHMANN*`
>  F5913499: all_macros.txt <https://reviews.llvm.org/F5913499>
>
> F5913498: filtered_macros.txt <https://reviews.llvm.org/F5913498>
>
> CMake on the other hand uses many macros and don't obey CAPS_ONLY rules for all macros.
>
> Filter:  `_FILE_OFFSET_BITS|AE_*|ARCHIVE_*|cal_*|CMAKE_*|cm*|CM_*|CPP_*|CURL*|E_*|exp_*|F90*|jp*|JSON_*|KW*|kw*|O_*|REQ_*|UV_*|YY*|yy*|Z_*`
>
> F5913502: warned_macros.txt <https://reviews.llvm.org/F5913502>
>
> F5913501: filtered_macros.txt <https://reviews.llvm.org/F5913501>
>
> 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*`
>
> F5913504: all_macros.txt <https://reviews.llvm.org/F5913504>
>
> F5913503: filtered_modules_macros.txt <https://reviews.llvm.org/F5913503>
>
> libtorrent https://github.com/arvidn/libtorrent has reasonable macro usage
>
> Filter: `DEBUG*|LIBTORRENT*|TORRENT*|UNI*`
>
> F5913519: filtered_macros.txt <https://reviews.llvm.org/F5913519>
>
> F5913518: all_macros.txt <https://reviews.llvm.org/F5913518>
>
> LLVM lib/ defines many macros, almost all of them are ALL_CAPS
>
> Filter: `AARCH64*|X86*|ARM*|AMDGPU*|ASAN*|ATTR*|CHECK*|COMMON*|CV*|CXX*|DEBUG*|DEC*|DEFINE*|ESAN*|ENUM*|FMA*|FUZZER*|HANDLE*|INTERCEPTOR*|LSAN*|MATH*|MSAN*|OPENMP*|TSAN*|TYPE*|UBSAN*`
>
> F5913505: all_lib_macros.txt <https://reviews.llvm.org/F5913505>
>  F5913528: filtered_lib_macros.txt <https://reviews.llvm.org/F5913528>
>
> LLVM tools/ is a similar story
>  Filter: `ANALYSIS*|ASSERT*|CHECK*|DEBUG*|DECL*|DEPENDENT*|DIAG*|END*|ENUM*|GENERIC*|IMAGE*|KEYWORD*|LANG*|LLVM*|NON*|OBJC*|OPENMP*|OVERLOAD*|PROC*|REGISTER*|SANITIZER*|STMT*|TYPE*|X86*`
>
> F5913566: all_tools_macros.txt <https://reviews.llvm.org/F5913566>
>
> F5913565: filtered_tools_macros.txt <https://reviews.llvm.org/F5913565>
>
> @aaron.ballman Would you like to see other projects checked? I think this is a starting point for now.
>
> My opinion based on what i see is
>
> - maybe two modes for this check make sense, having one requiring CAPS_ONLY and the currently implemented stricter check. This allows easier migration to safer macro usage
> - 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.
>
>   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.
> - enforcing ALL_CAPS, including its usage
> - (adding a prefix to all macro, passing the filter, including its usage)
>
>   Code transformation has the problems of scope and potentially breaking code badly, because clang-tidy wasn't run over all of the code.
>
> The check is chatty, as expected, because macros in header files, especially central ones, pollute everything. That is the reason for the check, too. 
>  Overall I am still in favor of this approach, seeing some obscure macros that should be replaced with actual language features (like overloading, .inline functions, constants, ...) in the checked codebases.


@aaron.ballman Did you have time to look at my analysis result?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41648