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

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp accepted this revision.
carlosgalvezp added a comment.
This revision is now accepted and ready to land.

Looks reasonable to me! I'm fairly new here so I might not be the most "authoritative reviewer", let me know if you'd like someone else to give the final approval



================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp:23
 
-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 inline bool isCapsOnly(StringRef Name) {
+  return llvm::all_of(Name, [](const char C) {
----------------
Nit: `inline` can be removed.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp:99
+  if (!Message.empty())
+    diag(MD->getLocation(), Message) << MacroName;
 }
----------------
Nit: tab with spaces.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-macro-usage.cpp:1
-// 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 --
 
----------------
I'm curious as to why this is needed. If I remove it the test fails, on line 15, but the `u8` prefix was introduced already since C++11?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116386/new/

https://reviews.llvm.org/D116386