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

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added inline comments.


================
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;
+inline bool isCapsOnly(StringRef Name) {
+  return llvm::all_of(Name, [](const char C) {
----------------
Shouldn't it still be "static"? "inline" will not give internal linkage. I think it's also unnecessary in this case since there's no risk for multiple definitions (it's not defined in a header file)


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst:11
+`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>`_.
+
----------------
Is ES.32 really checked by this check? I don't see any example or test that indicates that.

I'm also unsure if ES.32 should be handled here or via the "readability-identifier-naming" check, which is where you enforce a particular naming convention for different identifiers. Setting ALL_CAPS for macros there would be an effective way of solving ES.32.


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

https://reviews.llvm.org/D116386