aaron.ballman 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_*` This is a pretty long list of macro patterns to have to filter, but doesn't look to problematic. > 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*` 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. > 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. I think this is a good starting point. It's certainly demonstrated that even clean projects cannot conform to the C++ Core Guidelines. > 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 I think I agree. > - 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? 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. > 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? > - enforcing ALL_CAPS, including its usage What does "including its usage" mean? > - (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. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D41648 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
