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

lebedev.ri added inline comments. Comment at: clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp:67 + + diag(MD->getLocation(), DiagnosticMessage); +} And once again, i'm going to bitch about useless diagnostic messages: ``` [1/357] Building CXX object src/CMakeF

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

This revision was automatically updated to reflect the committed changes. Closed by commit rCTE344940: [clang-tidy] implement cppcoreguidelines macro rules (authored by JonasToth, committed by ). Changed prior to commit: https://reviews.llvm.org/D41648?vs=170472&id=170473#toc Repository: rCT

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

JonasToth updated this revision to Diff 170472. JonasToth added a comment. - [Doc] add missing release notes Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D41648 Files: clang-tidy/cppcoreguidelines/CMakeLists.txt clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp

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

JonasToth updated this revision to Diff 170471. JonasToth added a comment. - add missing private Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D41648 Files: clang-tidy/cppcoreguidelines/CMakeLists.txt clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp clang-tidy

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

JonasToth updated this revision to Diff 170470. JonasToth marked 4 inline comments as done. JonasToth added a comment. - address review nits Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D41648 Files: clang-tidy/cppcoreguidelines/CMakeLists.txt clang-tidy/cppcoreguidelines/

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

JonasToth added inline comments. Comment at: clang-tidy/cppcoreguidelines/MacroUsageCheck.h:34-35 + void registerPPCallbacks(CompilerInstance &Compiler) override; + void warnMacro(const MacroDirective *MD); + void warnNaming(const MacroDirective *MD); + aaron.

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

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM aside from some minor nits. Comment at: clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp:71 +void MacroUsageCheck::warnMacro(const MacroDirective *MD) { +

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

JonasToth updated this revision to Diff 169410. JonasToth added a comment. - make the logic with variadic clear Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D41648 Files: clang-tidy/cppcoreguidelines/CMakeLists.txt clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.c

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

JonasToth updated this revision to Diff 169403. JonasToth added a comment. - remove unused enum in header file, no idea what i intended to do with it :D Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D41648 Files: clang-tidy/cppcoreguidelines/CMakeLists.txt clang-tidy/cppcor

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

JonasToth updated this revision to Diff 169402. JonasToth marked 3 inline comments as done. JonasToth added a comment. - add tests and adjust doc - one more test case Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D41648 Files: clang-tidy/cppcoreguidelines/CMakeLists.txt cla

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

JonasToth added a comment. Updated Comment at: clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp:78 +"function-like macro used; consider a 'constexpr' template function"; + if (Info->isVariadic()) +DiagnosticMessage = "variadic macro used; consider using a 'constexp

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

aaron.ballman added a comment. In https://reviews.llvm.org/D41648#1253647, @JonasToth wrote: > @aaron.ballman What do you think of the current version of this check? > As migration strategy the option for `CAPS_ONLY` is provided, otherwise the > filtering is provided to silence necessary macros

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

JonasToth added a comment. @aaron.ballman What do you think of the current version of this check? As migration strategy the option for `CAPS_ONLY` is provided, otherwise the filtering is provided to silence necessary macros (which kind of enforces a common prefix for macros). This does implement

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

JonasToth updated this revision to Diff 168095. JonasToth added a comment. - add more positive tests - Merge branch 'master' into check_macros_usage Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D41648 Files: clang-tidy/cppcoreguidelines/CMakeLists.txt clang-tidy/cppcoregui

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

JonasToth updated this revision to Diff 168094. JonasToth added a comment. - Merge branch 'master' into check_macros_usage Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D41648 Files: clang-tidy/cppcoreguidelines/CMakeLists.txt clang-tidy/cppcoreguidelines/CppCoreGuidelinesT

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

JonasToth updated this revision to Diff 164948. JonasToth marked 7 inline comments as done. JonasToth added a comment. - Merge branch 'master' into check_macros_usage - address review comments Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D41648 Files: clang-tidy/cppcoreguide

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

JonasToth updated this revision to Diff 162450. JonasToth added a comment. - Merge branch 'master' into check_macros_usage Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D41648 Files: clang-tidy/cppcoreguidelines/CMakeLists.txt clang-tidy/cppcoreguidelines/CppCoreGuidelinesT

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

aaron.ballman added inline comments. Comment at: clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp:77 +DiagnosticMessage = +"function like macro used; consider a (constexpr) template function"; + if (Info->isVariadic()) function like -> function-like

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

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 mor

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

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

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

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 htt

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

JonasToth added inline comments. Comment at: docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst:24 + +.. option:: CheckCapsOnly + Eugene.Zelenko wrote: > Will be good idea to generalize this option with other check which deal with > macros. I dont think th

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

Eugene.Zelenko added a comment. In https://reviews.llvm.org/D41648#1048312, @JonasToth wrote: > Which checks do you have in mind? bugprone-macro-*, bugprone-multiple-statement-macro. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D41648 _

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

JonasToth added a comment. Which checks do you have in mind? Am 26.03.2018 um 18:07 schrieb Eugene Zelenko via Phabricator: > Eugene.Zelenko added inline comments. > > > Comment at: docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst:24 > + > +.. option:: CheckCapsOnly

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

Eugene.Zelenko added inline comments. Comment at: docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst:24 + +.. option:: CheckCapsOnly + Will be good idea to generalize this option with other check which deal with macros. Repository: rCTE Clang Tools Ext

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

Eugene.Zelenko added inline comments. Comment at: docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst:26 + +This boolean flag disables all warnings for macros and checks only that +macro names are CAPS_ONLY. This option is meant to ease introduction of this ---

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

JonasToth updated this revision to Diff 139793. JonasToth added a comment. - [Feature] implement CAPS_ONLY features, adjust docs Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D41648 Files: clang-tidy/cppcoreguidelines/CMakeLists.txt clang-tidy/cppcoreguidelines/CppCoreGuide

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

JonasToth added a comment. 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

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

JonasToth updated this revision to Diff 139707. JonasToth added a comment. - Merge branch 'master' into check_macros_usage - [Doc] update to new doc style Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D41648 Files: clang-tidy/cppcoreguidelines/CMakeLists.txt clang-tidy/cppc

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

JonasToth added a comment. In https://reviews.llvm.org/D41648#976665, @aaron.ballman wrote: > My gut reaction is still that this check is going to be too chatty on real > world code bases to be of value beyond compliance with the C++ Core > Guidelines, but that's also sufficient enough reason t

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

aaron.ballman added a comment. My gut reaction is still that this check is going to be too chatty on real world code bases to be of value beyond compliance with the C++ Core Guidelines, but that's also sufficient enough reason to have the check in the first place. Perhaps a reasonable approach

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

JonasToth added a comment. > 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 th

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

JonasToth updated this revision to Diff 129893. JonasToth added a comment. - address eugene comments Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D41648 Files: clang-tidy/cppcoreguidelines/CMakeLists.txt clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp clang-

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

JonasToth marked 2 inline comments as done. JonasToth added inline comments. 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

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

JonasToth updated this revision to Diff 129890. JonasToth marked 2 inline comments as done. JonasToth added a comment. - doc fix Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D41648 Files: clang-tidy/cppcoreguidelines/CMakeLists.txt clang-tidy/cppcoreguidelines/CppCoreGuide

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

JonasToth updated this revision to Diff 129889. JonasToth marked 2 inline comments as done. JonasToth added a comment. - address review comments Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D41648 Files: clang-tidy/cppcoreguidelines/CMakeLists.txt clang-tidy/cppcoreguideli

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

Eugene.Zelenko added inline comments. Comment at: clang-tidy/cppcoreguidelines/MacroUsageCheck.h:15 +#include "clang/Lex/Preprocessor.h" + +namespace clang { Please include . Comment at: docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst:

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

JonasToth updated this revision to Diff 129886. JonasToth added a comment. - [Feature] implement regex for allowed macros - [Test] add proper tests for regex silencing Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D41648 Files: clang-tidy/cppcoreguidelines/CMakeLists.txt cl

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

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

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

JonasToth updated this revision to Diff 129691. JonasToth added a comment. - minor issues fixed Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D41648 Files: clang-tidy/cppcoreguidelines/CMakeLists.txt clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp clang-tidy/

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

JonasToth updated this revision to Diff 129690. JonasToth marked 3 inline comments as done. JonasToth added a comment. - address review comments Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D41648 Files: clang-tidy/cppcoreguidelines/CMakeLists.txt clang-tidy/cppcoreguideli

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

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 ba

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

JonasToth updated this revision to Diff 128869. JonasToth added a comment. - rebase after release for 6.0 Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D41648 Files: clang-tidy/cppcoreguidelines/CMakeLists.txt clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp c

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

Eugene.Zelenko added inline comments. Comment at: clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp:13 +#include "clang/Lex/PPCallbacks.h" + +namespace clang { Please include string and STLExtras.h. Comment at: docs/clang-tidy/checks/cppcoreguid

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

aaron.ballman added a comment. In https://reviews.llvm.org/D41648#965437, @JonasToth wrote: > In https://reviews.llvm.org/D41648#965432, @aaron.ballman wrote: > > > I think this check is going to be extraordinarily chatty. For instance, > > macros are often used to hide compiler details in signa

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

JonasToth added a comment. In https://reviews.llvm.org/D41648#965432, @aaron.ballman wrote: > I think this check is going to be extraordinarily chatty. For instance, > macros are often used to hide compiler details in signatures, such as use of > attributes. This construct cannot be replaced wi

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

aaron.ballman added a comment. I think this check is going to be extraordinarily chatty. For instance, macros are often used to hide compiler details in signatures, such as use of attributes. This construct cannot be replaced with anything else because the macro isn't defining an object or valu

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

JonasToth updated this revision to Diff 128373. JonasToth added a comment. - remove unneeded includes Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D41648 Files: clang-tidy/cppcoreguidelines/CMakeLists.txt clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp clang

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

JonasToth updated this revision to Diff 128372. JonasToth added a comment. - merge with local repos Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D41648 Files: clang-tidy/cppcoreguidelines/CMakeLists.txt clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp clang-t

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

JonasToth created this revision. JonasToth added reviewers: aaron.ballman, hokein, alexfh. Herald added subscribers: cfe-commits, kbarton, xazax.hun, mgorny, nemanjai, klimek. In short macros are discouraged by multiple rules (and sometimes reference randomly). This check allows only headerguard