[clang-tidy] Narrow cppguidelines-macro-usage to actual constants

Previously, any macro that didn't look like a varargs macro
or a function style macro was reported with a warning that
it should be replaced with a constexpr const declaration.
This is only reasonable when the macro body contains constants
and not expansions like ",", "[[noreturn]]", "__declspec(xxx)",
etc.

So instead of always issuing a warning about every macro that
doesn't look like a varargs or function style macro, examine the
tokens in the macro and only warn about the macro if it contains
only comment and constant tokens.

Differential Revision: https://reviews.llvm.org/D116386

Fixes #39945

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

@@ -14,25 +14,25 @@
 #include "llvm/Support/Regex.h"
 #include <algorithm>
 #include <cctype>
+#include <functional>
 
 namespace clang {
 namespace tidy {
 namespace cppcoreguidelines {
 
-namespace {
-
-bool isCapsOnly(StringRef Name) {
-  return std::all_of(Name.begin(), Name.end(), [](const char C) {
-    if (std::isupper(C) || std::isdigit(C) || C == '_')
-      return true;
-    return false;
+static bool isCapsOnly(StringRef Name) {
+  return llvm::all_of(Name, [](const char C) {
+    return std::isupper(C) || std::isdigit(C) || C == '_';
   });
 }
 
+namespace {
+
 class MacroUsageCallbacks : public PPCallbacks {
 public:
   MacroUsageCallbacks(MacroUsageCheck *Check, const SourceManager &SM,
-                      StringRef RegExpStr, bool CapsOnly, bool IgnoreCommandLine)
+                      StringRef RegExpStr, bool CapsOnly,
+                      bool IgnoreCommandLine)
       : Check(Check), SM(SM), RegExp(RegExpStr), CheckCapsOnly(CapsOnly),
         IgnoreCommandLineMacros(IgnoreCommandLine) {}
   void MacroDefined(const Token &MacroNameTok,
@@ -79,21 +79,24 @@ void MacroUsageCheck::registerPPCallbacks(const SourceManager &SM,
 }
 
 void MacroUsageCheck::warnMacro(const MacroDirective *MD, StringRef MacroName) {
-  StringRef Message =
-      "macro '%0' used to declare a constant; consider using a 'constexpr' "
-      "constant";
+  const MacroInfo *Info = MD->getMacroInfo();
+  StringRef Message;
 
-  /// 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())
+  if (llvm::all_of(Info->tokens(), std::mem_fn(&Token::isLiteral)))
+    Message = "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.
+  else if (Info->isVariadic())
     Message = "variadic macro '%0' used; consider using a 'constexpr' "
               "variadic template function";
-  else if (MD->getMacroInfo()->isFunctionLike())
+  else if (Info->isFunctionLike())
     Message = "function-like macro '%0' used; consider a 'constexpr' template "
               "function";
 
-  diag(MD->getLocation(), Message) << MacroName;
+  if (!Message.empty())
+    diag(MD->getLocation(), Message) << MacroName;
 }
 
 void MacroUsageCheck::warnNaming(const MacroDirective *MD,

clang-tools-extra/docs/ReleaseNotes.rst

@@ -83,6 +83,9 @@ Improvements to clang-tidy
 - Generalized the `modernize-use-default-member-init` check to handle non-default
   constructors.
 
+- Eliminated false positives for `cppcoreguidelines-macro-usage` by restricting
+  the warning about using constants to only macros that expand to literals.
+
 New checks
 ^^^^^^^^^^
 

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

@@ -7,10 +7,40 @@ Finds macro usage that is considered problematic because better language
 constructs exist for the task.
 
 The relevant sections in the C++ Core Guidelines are
-`Enum.1 <https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#enum1-prefer-enumerations-over-macros>`_,
-`ES.30 <https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es30-dont-use-macros-for-program-text-manipulation>`_,
-`ES.31 <https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es31-dont-use-macros-for-constants-or-functions>`_ and
-`ES.33 <https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es33-if-you-must-use-macros-give-them-unique-names>`_.
+`ES.31 <https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es31-dont-use-macros-for-constants-or-functions>`_, and
+`ES.32 <https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es32-use-all_caps-for-all-macro-names>`_.
+
+Examples:
+
+.. code-block:: c++
+
+  #define C 0
+  #define F1(x, y) ((a) > (b) ? (a) : (b))
+  #define F2(...) (__VA_ARGS__)
+  #define COMMA ,
+  #define NORETURN [[noreturn]]
+  #define DEPRECATED attribute((deprecated))
+  #if LIB_EXPORTS
+  #define DLLEXPORTS __declspec(dllexport)
+  #else
+  #define DLLEXPORTS __declspec(dllimport)
+  #endif
+
+results in the following warnings:
+
+.. code-block:: c++
+
+  4 warnings generated.
+  test.cpp:1:9: warning: macro 'C' used to declare a constant; consider using a 'constexpr' constant [cppcoreguidelines-macro-usage]
+  #define C 0
+          ^
+  test.cpp:2:9: warning: function-like macro 'F1' used; consider a 'constexpr' template function [cppcoreguidelines-macro-usage]
+  #define F1(x, y) ((a) > (b) ? (a) : (b))
+          ^
+  test.cpp:3:9: warning: variadic macro 'F2' used; consider using a 'constexpr' variadic template function [cppcoreguidelines-macro-usage]
+  #define F2(...) (__VA_ARGS__)
+          ^
+
 
 Options
 -------

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

@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t -- -header-filter=.* -system-headers --
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage -std=c++17-or-later %t -- -header-filter=.* -system-headers --
 
 #ifndef INCLUDE_GUARD
 #define INCLUDE_GUARD
@@ -6,6 +6,21 @@
 #define PROBLEMATIC_CONSTANT 0
 // CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro 'PROBLEMATIC_CONSTANT' used to declare a constant; consider using a 'constexpr' constant
 
+#define PROBLEMATIC_CONSTANT_CHAR '0'
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro 'PROBLEMATIC_CONSTANT_CHAR' used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_CONSTANT_WIDE_CHAR L'0'
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro 'PROBLEMATIC_CONSTANT_WIDE_CHAR' used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_CONSTANT_UTF8_CHAR u8'0'
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro 'PROBLEMATIC_CONSTANT_UTF8_CHAR' used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_CONSTANT_UTF16_CHAR u'0'
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro 'PROBLEMATIC_CONSTANT_UTF16_CHAR' used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_CONSTANT_UTF32_CHAR U'0'
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro 'PROBLEMATIC_CONSTANT_UTF32_CHAR' 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 'PROBLEMATIC_FUNCTION' used; consider a 'constexpr' template function
 
@@ -15,4 +30,17 @@
 #define PROBLEMATIC_VARIADIC2(x, ...) (__VA_ARGS__)
 // CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro 'PROBLEMATIC_VARIADIC2' used; consider using a 'constexpr' variadic template function
 
+// These are all examples of common macros that shouldn't have constexpr suggestions.
+#define COMMA ,
+
+#define NORETURN [[noreturn]]
+
+#define DEPRECATED attribute((deprecated))
+
+#if LIB_EXPORTS
+#define DLLEXPORTS __declspec(dllexport)
+#else
+#define DLLEXPORTS __declspec(dllimport)
+#endif
+
 #endif