From c367ba1923335a90e760923dd8b93164ff9af22f Mon Sep 17 00:00:00 2001 From: Roman Lebedev Date: Tue, 30 Oct 2018 15:52:36 +0000 Subject: [PATCH] [clang-tidy] cppcoreguidelines-macro-usage: print macro names Summary: The macro may not have location (or more generally, the location may not exist), e.g. if it originates from compiler's command-line. The check complains on all the macros, even those without the location info. Which means, it only says it does not like it. What is 'it'? I have no idea. If we don't print the name, then there is no way to deal with that situation. And in general, not printing name here forces the user to try to understand, given, the macro definition location, what is the macro name? This isn't fun. Also, ignores-by-default the macros originating from command-line, with an option to not ignore those. I suspect some more issues may crop up later. Reviewers: JonasToth, aaron.ballman, hokein, xazax.hun, alexfh Reviewed By: JonasToth, aaron.ballman Subscribers: nemanjai, kbarton, rnkovacs, cfe-commits Tags: #clang-tools-extra Differential Revision: https://reviews.llvm.org/D53817 llvm-svn: 345610 --- .../cppcoreguidelines/MacroUsageCheck.cpp | 38 ++++++++++++++-------- .../clang-tidy/cppcoreguidelines/MacroUsageCheck.h | 9 +++-- .../checks/cppcoreguidelines-macro-usage.rst | 5 +++ .../cppcoreguidelines-macro-usage-caps-only.cpp | 8 ++--- ...eguidelines-macro-usage-command-line-macros.cpp | 8 +++++ .../cppcoreguidelines-macro-usage-custom.cpp | 8 ++--- .../clang-tidy/cppcoreguidelines-macro-usage.cpp | 8 ++--- 7 files changed, 56 insertions(+), 28 deletions(-) create mode 100644 clang-tools-extra/test/clang-tidy/cppcoreguidelines-macro-usage-command-line-macros.cpp diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp index 47f563b..8b893c7 100644 --- a/clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp @@ -31,32 +31,41 @@ bool isCapsOnly(StringRef Name) { class MacroUsageCallbacks : public PPCallbacks { public: - MacroUsageCallbacks(MacroUsageCheck *Check, StringRef RegExp, bool CapsOnly) - : Check(Check), RegExp(RegExp), CheckCapsOnly(CapsOnly) {} + MacroUsageCallbacks(MacroUsageCheck *Check, const SourceManager &SM, + StringRef RegExp, bool CapsOnly, bool IgnoreCommandLine) + : Check(Check), SM(SM), RegExp(RegExp), CheckCapsOnly(CapsOnly), + IgnoreCommandLineMacros(IgnoreCommandLine) {} void MacroDefined(const Token &MacroNameTok, const MacroDirective *MD) override { if (MD->getMacroInfo()->isUsedForHeaderGuard() || MD->getMacroInfo()->getNumTokens() == 0) return; + if (IgnoreCommandLineMacros && + SM.isWrittenInCommandLineFile(MD->getLocation())) + return; + StringRef MacroName = MacroNameTok.getIdentifierInfo()->getName(); if (!CheckCapsOnly && !llvm::Regex(RegExp).match(MacroName)) - Check->warnMacro(MD); + Check->warnMacro(MD, MacroName); if (CheckCapsOnly && !isCapsOnly(MacroName)) - Check->warnNaming(MD); + Check->warnNaming(MD, MacroName); } private: MacroUsageCheck *Check; + const SourceManager &SM; StringRef RegExp; bool CheckCapsOnly; + bool IgnoreCommandLineMacros; }; } // namespace void MacroUsageCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { Options.store(Opts, "AllowedRegexp", AllowedRegexp); Options.store(Opts, "CheckCapsOnly", CheckCapsOnly); + Options.store(Opts, "IgnoreCommandLineMacros", IgnoreCommandLineMacros); } void MacroUsageCheck::registerPPCallbacks(CompilerInstance &Compiler) { @@ -64,31 +73,34 @@ void MacroUsageCheck::registerPPCallbacks(CompilerInstance &Compiler) { return; Compiler.getPreprocessor().addPPCallbacks( - llvm::make_unique(this, AllowedRegexp, - CheckCapsOnly)); + llvm::make_unique(this, Compiler.getSourceManager(), + AllowedRegexp, CheckCapsOnly, + IgnoreCommandLineMacros)); } -void MacroUsageCheck::warnMacro(const MacroDirective *MD) { +void MacroUsageCheck::warnMacro(const MacroDirective *MD, StringRef MacroName) { StringRef Message = - "macro used to declare a constant; consider using a 'constexpr' " + "macro '%0' used to declare a constant; consider using a 'constexpr' " "constant"; /// A variadic macro is function-like at the same time. Therefore variadic /// macros are checked first and will be excluded for the function-like /// diagnostic. if (MD->getMacroInfo()->isVariadic()) - Message = "variadic macro used; consider using a 'constexpr' " + Message = "variadic macro '%0' used; consider using a 'constexpr' " "variadic template function"; else if (MD->getMacroInfo()->isFunctionLike()) - Message = "function-like macro used; consider a 'constexpr' template " + Message = "function-like macro '%0' used; consider a 'constexpr' template " "function"; - diag(MD->getLocation(), Message); + diag(MD->getLocation(), Message) << MacroName; } -void MacroUsageCheck::warnNaming(const MacroDirective *MD) { +void MacroUsageCheck::warnNaming(const MacroDirective *MD, + StringRef MacroName) { diag(MD->getLocation(), "macro definition does not define the macro name " - "using all uppercase characters"); + "'%0' using all uppercase characters") + << MacroName; } } // namespace cppcoreguidelines diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.h b/clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.h index ff33310..a39d55a 100644 --- a/clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.h +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.h @@ -28,17 +28,20 @@ public: MacroUsageCheck(StringRef Name, ClangTidyContext *Context) : ClangTidyCheck(Name, Context), AllowedRegexp(Options.get("AllowedRegexp", "^DEBUG_*")), - CheckCapsOnly(Options.get("CheckCapsOnly", 0)) {} + CheckCapsOnly(Options.get("CheckCapsOnly", 0)), + IgnoreCommandLineMacros(Options.get("IgnoreCommandLineMacros", 1)) {} void storeOptions(ClangTidyOptions::OptionMap &Opts) override; void registerPPCallbacks(CompilerInstance &Compiler) override; - void warnMacro(const MacroDirective *MD); - void warnNaming(const MacroDirective *MD); + void warnMacro(const MacroDirective *MD, StringRef MacroName); + void warnNaming(const MacroDirective *MD, StringRef MacroName); private: /// A regular expression that defines how allowed macros must look like. std::string AllowedRegexp; /// Control if only the check shall only test on CAPS_ONLY macros. bool CheckCapsOnly; + /// Should the macros without a valid location be diagnosed? + bool IgnoreCommandLineMacros; }; } // namespace cppcoreguidelines diff --git a/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst b/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst index 4fe0b48..01cfbd6 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst @@ -26,3 +26,8 @@ Options Boolean flag to warn on all macros except those with CAPS_ONLY names. This option is intended to ease introduction of this check into older code bases. Default value is `0`/`false`. + +.. option:: IgnoreCommandLineMacros + + Boolean flag to toggle ignoring command-line-defined macros. + Default value is `1`/`true`. diff --git a/clang-tools-extra/test/clang-tidy/cppcoreguidelines-macro-usage-caps-only.cpp b/clang-tools-extra/test/clang-tidy/cppcoreguidelines-macro-usage-caps-only.cpp index db9a41a..a1d49bf 100644 --- a/clang-tools-extra/test/clang-tidy/cppcoreguidelines-macro-usage-caps-only.cpp +++ b/clang-tools-extra/test/clang-tidy/cppcoreguidelines-macro-usage-caps-only.cpp @@ -6,16 +6,16 @@ #define INCLUDE_GUARD #define problematic_constant 0 -// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro definition does not define the macro name using all uppercase characters +// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro definition does not define the macro name 'problematic_constant' using all uppercase characters #define problematic_function(x, y) ((a) > (b) ? (a) : (b)) -// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro definition does not define the macro name using all uppercase characters +// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro definition does not define the macro name 'problematic_function' using all uppercase characters #define problematic_variadic(...) (__VA_ARGS__) -// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro definition does not define the macro name using all uppercase characters +// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro definition does not define the macro name 'problematic_variadic' using all uppercase characters // #define problematic_variadic2(x, ...) (__VA_ARGS__) -// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro definition does not define the macro name using all uppercase characters +// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro definition does not define the macro name 'problematic_variadic2' using all uppercase characters #define OKISH_CONSTANT 42 #define OKISH_FUNCTION(x, y) ((a) > (b) ? (a) : (b)) diff --git a/clang-tools-extra/test/clang-tidy/cppcoreguidelines-macro-usage-command-line-macros.cpp b/clang-tools-extra/test/clang-tidy/cppcoreguidelines-macro-usage-command-line-macros.cpp new file mode 100644 index 0000000..d4f9a0d --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/cppcoreguidelines-macro-usage-command-line-macros.cpp @@ -0,0 +1,8 @@ +// RUN: %check_clang_tidy -check-suffixes=NORMAL %s cppcoreguidelines-macro-usage %t -- -- -D_ZZZ_IM_A_MACRO +// RUN: %check_clang_tidy -check-suffixes=NORMAL %s cppcoreguidelines-macro-usage %t -- -config='{CheckOptions: [{key: cppcoreguidelines-macro-usage.IgnoreCommandLineMacros, value: 1}]}' -- -D_ZZZ_IM_A_MACRO +// RUN: %check_clang_tidy -check-suffixes=NORMAL,CL %s cppcoreguidelines-macro-usage %t -- -config='{CheckOptions: [{key: cppcoreguidelines-macro-usage.IgnoreCommandLineMacros, value: 0}]}' -- -D_ZZZ_IM_A_MACRO + +// CHECK-MESSAGES-CL: warning: macro '_ZZZ_IM_A_MACRO' used to declare a constant; consider using a 'constexpr' constant + +#define PROBLEMATIC_CONSTANT 0 +// CHECK-MESSAGES-NORMAL: [[@LINE-1]]:9: warning: macro 'PROBLEMATIC_CONSTANT' used to declare a constant; consider using a 'constexpr' constant diff --git a/clang-tools-extra/test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp b/clang-tools-extra/test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp index 03e0724..e25eae3 100644 --- a/clang-tools-extra/test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp +++ b/clang-tools-extra/test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp @@ -6,16 +6,16 @@ #define INCLUDE_GUARD #define PROBLEMATIC_CONSTANT 0 -// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro used to declare a constant; consider using a 'constexpr' constant +// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro 'PROBLEMATIC_CONSTANT' used to declare a constant; consider using a 'constexpr' constant #define PROBLEMATIC_FUNCTION(x, y) ((a) > (b) ? (a) : (b)) -// CHECK-MESSAGES: [[@LINE-1]]:9: warning: function-like macro used; consider a 'constexpr' template function +// CHECK-MESSAGES: [[@LINE-1]]:9: warning: function-like macro 'PROBLEMATIC_FUNCTION' used; consider a 'constexpr' template function #define PROBLEMATIC_VARIADIC(...) (__VA_ARGS__) -// CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro used; consider using a 'constexpr' variadic template function +// CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro 'PROBLEMATIC_VARIADIC' used; consider using a 'constexpr' variadic template function #define PROBLEMATIC_VARIADIC2(x, ...) (__VA_ARGS__) -// CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro used; consider using a 'constexpr' variadic template function +// CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro 'PROBLEMATIC_VARIADIC2' used; consider using a 'constexpr' variadic template function #define DEBUG_CONSTANT 0 #define DEBUG_FUNCTION(x, y) ((a) > (b) ? (a) : (b)) diff --git a/clang-tools-extra/test/clang-tidy/cppcoreguidelines-macro-usage.cpp b/clang-tools-extra/test/clang-tidy/cppcoreguidelines-macro-usage.cpp index ea92e19..a4948f5b 100644 --- a/clang-tools-extra/test/clang-tidy/cppcoreguidelines-macro-usage.cpp +++ b/clang-tools-extra/test/clang-tidy/cppcoreguidelines-macro-usage.cpp @@ -4,15 +4,15 @@ #define INCLUDE_GUARD #define PROBLEMATIC_CONSTANT 0 -// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro used to declare a constant; consider using a 'constexpr' constant +// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro 'PROBLEMATIC_CONSTANT' used to declare a constant; consider using a 'constexpr' constant #define PROBLEMATIC_FUNCTION(x, y) ((a) > (b) ? (a) : (b)) -// CHECK-MESSAGES: [[@LINE-1]]:9: warning: function-like macro used; consider a 'constexpr' template function +// CHECK-MESSAGES: [[@LINE-1]]:9: warning: function-like macro 'PROBLEMATIC_FUNCTION' used; consider a 'constexpr' template function #define PROBLEMATIC_VARIADIC(...) (__VA_ARGS__) -// CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro used; consider using a 'constexpr' variadic template function +// CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro 'PROBLEMATIC_VARIADIC' used; consider using a 'constexpr' variadic template function #define PROBLEMATIC_VARIADIC2(x, ...) (__VA_ARGS__) -// CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro used; consider using a 'constexpr' variadic template function +// CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro 'PROBLEMATIC_VARIADIC2' used; consider using a 'constexpr' variadic template function #endif -- 2.7.4