[clang-tidy] cppcoreguidelines-macro-usage: print macro names

[lebedev.ri]

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.

I suspect some more issues may crop up later.

[JonasToth]

How does this patch change the behaviour for macros without location? They will be diagnosed at the beginning of the TU?

It is probably better to ignore these kind of macros, as there is no way around them (compile time configuration can only be done with macros?!) and therefore warnings for theses macros are false positives. Using constexpr constructs does not help either.

[lebedev.ri]

In D53817#1278916, @JonasToth wrote:

How does this patch change the behaviour for macros without location?

It doesn't.

They will be diagnosed at the beginning of the TU?

It is probably better to ignore these kind of macros, as there is no way around them (compile time configuration can only be done with macros?!) and therefore warnings for theses macros are false positives. Using constexpr constructs does not help either.

I agree that it might be good idea to add an *option* to not diagnose such macros.
But i think it is not the spirit of the check/rule to ignore them completely [by default],
because those macros are still horrible, and can affect all kinds of things,
just like the 'normal' macros.

[JonasToth]

They will be diagnosed at the beginning of the TU?
It is probably better to ignore these kind of macros, as there is no way around them (compile time configuration can only be done with macros?!) and therefore warnings for theses macros are false positives. Using constexpr constructs does not help either.

I agree that it might be good idea to add an *option* to not diagnose such macros.
But i think it is not the spirit of the check/rule to ignore them completely [by default],
because those macros are still horrible, and can affect all kinds of things,
just like the 'normal' macros.

Yes and no, the CPPCG do acknowledge the fact, that macros are (still)
necessary for some jobs, like compile-time configurations of values and
require that the macros should at least be super visible in the code
(enforceable with the regex option in this check).

Turning off assertions for release builds would be a good example of
necessary macros. I would say this check must take this into account.

As for printing the name in the diagnostic: Not sure if this really adds
value. The location the diagnostic is pointing to is the name of macro
itself. The unclear diag comes from the fact that the macro is not
defined in code, so i think we should find a position on that issue first.

My opinion is, that macros without location (however compiler defined
macros) should not be diagnosed. It is unlikely that complicated
functions or the like a defined via compiler flags. These tend to be
used for integral constants to turn things on/off.

[aaron.ballman]

In D53817#1278933, @lebedev.ri wrote:

In D53817#1278916, @JonasToth wrote:

How does this patch change the behaviour for macros without location?

It doesn't.

They will be diagnosed at the beginning of the TU?

It is probably better to ignore these kind of macros, as there is no way around them (compile time configuration can only be done with macros?!) and therefore warnings for theses macros are false positives. Using constexpr constructs does not help either.

I agree that it might be good idea to add an *option* to not diagnose such macros.
But i think it is not the spirit of the check/rule to ignore them completely [by default],
because those macros are still horrible, and can affect all kinds of things,
just like the 'normal' macros.

I'm not certain an option makes sense in practice. Predefined macros and macros that come from the command line are a different beast from macros defined by the developer -- I don't think we should be diagnosing those cases as they're generally outside of programmer control. Predefined macros are entirely outside of the user's control. Macros from the command line are most often going to be controlling build toggles, but there is no reason to assume the user can control those (like 3rd party packages) or that they can be replaced with an alternative like constexpr.

[lebedev.ri]

In D53817#1278970, @aaron.ballman wrote:

In D53817#1278933, @lebedev.ri wrote:

In D53817#1278916, @JonasToth wrote:

How does this patch change the behaviour for macros without location?

It doesn't.

They will be diagnosed at the beginning of the TU?

It is probably better to ignore these kind of macros, as there is no way around them (compile time configuration can only be done with macros?!) and therefore warnings for theses macros are false positives. Using constexpr constructs does not help either.

I agree that it might be good idea to add an *option* to not diagnose such macros.
But i think it is not the spirit of the check/rule to ignore them completely [by default],
because those macros are still horrible, and can affect all kinds of things,
just like the 'normal' macros.

I'm not certain an option makes sense in practice. Predefined macros and macros that come from the command line are a different beast from macros defined by the developer -- I don't think we should be diagnosing those cases as they're generally outside of programmer control. Predefined macros are entirely outside of the user's control. Macros from the command line are most often going to be controlling build toggles, but there is no reason to assume the user can control those (like 3rd party packages) or that they can be replaced with an alternative like constexpr.

I personally haven't yet seen this check firing on *predefined* macros.
The only problem i have are these command-line-based macros.
They *may* be outside of programmer's control, but if one has access to the buildsystem
(as in, not *just* to the source code to be compiled, but to the build system definitions
that say *how* to build it), then one can easily add such command-line-based macros.

[JonasToth]

I personally haven't yet seen this check firing on *predefined* macros.

I did specifically check the compiler defined macros I could see in the AST and they were not diagnosed from the beginning.
I would guess, only the compiler-flag macros are passed in the PPCallbacks, which would explain the current behaviour.

The only problem i have are these command-line-based macros.
They *may* be outside of programmer's control, but if one has access to the buildsystem
(as in, not *just* to the source code to be compiled, but to the build system definitions
that say *how* to build it), then one can easily add such command-line-based macros.

It is a burden with little to gain as they the toggle-stuff use-case is not replaceable with other constructs.
Having it as opt-in might be ok, but by default the check should not diagnose them.

[aaron.ballman]

In D53817#1279053, @JonasToth wrote:

I personally haven't yet seen this check firing on *predefined* macros.

I did specifically check the compiler defined macros I could see in the AST and they were not diagnosed from the beginning.
I would guess, only the compiler-flag macros are passed in the PPCallbacks, which would explain the current behaviour.

Thanks for checking; I hadn't had the chance to try it out yet and was making a guess.

The only problem i have are these command-line-based macros.
They *may* be outside of programmer's control, but if one has access to the buildsystem
(as in, not *just* to the source code to be compiled, but to the build system definitions
that say *how* to build it), then one can easily add such command-line-based macros.

It is a burden with little to gain as they the toggle-stuff use-case is not replaceable with other constructs.
Having it as opt-in might be ok, but by default the check should not diagnose them.

+1; I'd be fine with an opt-in option for the behavior.

[lebedev.ri]

Add an opt-in option to diagnose such command-line-based macros.

[JonasToth]

docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst

30 ↗

(On Diff #171583)

I think the another option name would fit better: how about IgnoreCommandLineMacros or the like?

[lebedev.ri]

Better option name.

docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst

30 ↗

(On Diff #171583)

Yep, i didn't like that name anyway :)

[JonasToth]

LGTM, @aaron.ballman do you have more comments?

[lebedev.ri]

In D53817#1280405, @JonasToth wrote:

LGTM

Thank you for the review!

@aaron.ballman do you have more comments?

If there are no more comments, i'll land this in a few (~3) hours.

[aaron.ballman]

LGTM!

clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/MacroUsageCheck.h

 ///
 /// For the user-facing documentation see:
 /// http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-macro-usage.html
 class MacroUsageCheck : public ClangTidyCheck {
 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
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_MACROUSAGECHECK_H

clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp

return std::all_of(Name.begin(), Name.end(), [](const char c) {
     if (std::isupper(c) || std::isdigit(c) || c == '_')
       return true;
     return false;
   });
 }
 
 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) {
   if (!getLangOpts().CPlusPlus11)
     return;
 
   Compiler.getPreprocessor().addPPCallbacks(
-      llvm::make_unique<MacroUsageCallbacks>(this, AllowedRegexp,
-                                             CheckCapsOnly));
+      llvm::make_unique<MacroUsageCallbacks>(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
 } // namespace tidy
 } // namespace clang

clang-tools-extra/trunk/docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst

.. option:: AllowedRegexp
     `DEBUG*|LIBTORRENT*|TORRENT*|UNI*` could be applied to filter `libtorrent`.
     Default value is `^DEBUG_*`.
 
 .. option:: CheckCapsOnly
 
     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`.

clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-macro-usage-caps-only.cpp

 // RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t \
 // RUN: -config='{CheckOptions: \
 // RUN:  [{key: cppcoreguidelines-macro-usage.CheckCapsOnly, value: 1}]}' --
 
 #ifndef INCLUDE_GUARD
 #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))
 #define OKISH_VARIADIC(...) (__VA_ARGS__)
 
 #endif

clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-macro-usage-command-line-macros.cpp

+// 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

clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp

 // RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t \
 // RUN: -config='{CheckOptions: \
 // RUN:  [{key: cppcoreguidelines-macro-usage.AllowedRegexp, value: "DEBUG_*|TEST_*"}]}' --
 
 #ifndef INCLUDE_GUARD
 #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))
 #define DEBUG_VARIADIC(...) (__VA_ARGS__)
 #define TEST_CONSTANT 0
 #define TEST_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
 #define TEST_VARIADIC(...) (__VA_ARGS__)
 #define TEST_VARIADIC2(x, ...) (__VA_ARGS__)
 
 #endif

clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-macro-usage.cpp

 // RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t
 
 #ifndef INCLUDE_GUARD
 #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