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

[LegalizeAdulthood]

Summary

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.

Fixes #39945

[LegalizeAdulthood]

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

83–105

In the spirit of "avoid raw loops", would this be clearer expressed as std::all_of and the switch/case provided as a lambda?

[LegalizeAdulthood]

Improve name of predicate function.
Use standard algorithms instead of raw loops

[LegalizeAdulthood]

Can someone look at this review please, it's not that long and/or complicated. Thanks.

[aaron.ballman]

We run into this problem quite frequently -- the C++ Core Guidelines put very little effort into thinking about enforcement, and so tool vendors like us are left holding the bag. We can either do what the rule says for enforcement (which is what users often expect to happen, because the rules are supposed to be the source of truth), or we can do something actually useful. For example, this rule is documented (https://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-macro-usage.html) to cover ES.30 and ES.31 whose enforcement is Scream when you see a macro that isn't just used for source control (e.g., #ifdef). When the rule and the check disagree, we usually ask check authors to work with the guideline authors to come to an agreement on what the rule should be changed to say and I think we need to do that dance again here. (Aside: our docs also claim this checks for ES.33 whose enforcement is Warn against short macro names. and I don't see anything in the check that actually does that; I think the docs meant ES.32 about defining macro names in all caps, and the option for this part of the check is not enabled by default.)

However, my personal experience on engaging with the C++ Core Guidelines authors on enforcement issues in the past has not been positive, which is why I made a personal decision to not review C++ Core Guidelines for clang-tidy once they started hitting enforcement issues where the rule is of too low quality (I don't have the time to do that amount of work on behalf of the C++ Core Guidelines). I think that's the case here, so I'm resigning from the review. However, if the C++ Core Guidelines authors engage in the topic and improve their enforcement to be usefully enforceable, I'm happy to be added back as a reviewer.

I'm adding a few new reviewers who might have more ability to engage on the review in the near term.

[LegalizeAdulthood]

Aaron, I think your comments are useful and I would be inclined to agree with you if I
was the original author of this check. I treat the guidelines as just that: guidelines,
not rules. In the context of clang-tidy I think you're correct that some guidelines
are easily turned into usable diagnostics and a subset of those can become enforceable
rules with suggested fixits.

In this case, the check only issues diagnostics, not fixits. When the diagnostics result
in many false positives (as per the open bug), then I think it's reasonable to narrow the
scope of the check to omit the false positives.

The worst thing a "guideline" checker can do is to constantly nag you about false
positives. This trains people to not run the checkers and/or ignore all their complaints.

[LegalizeAdulthood]

Also, re-reading the C++ guidelines, I agree that this check is handling the two cases
ES.31 (Don't use macros for constants and functions)
ES.33 (Don't use macros that aren't all caps)

I guess previously it technically handled ES.30 (don't use macros for text manipulation)
by just complaining about every macro not being replaced with a constexpr constant,
LOL.

[aaron.ballman]

In D116386#3226174, @LegalizeAdulthood wrote:

Aaron, I think your comments are useful and I would be inclined to agree with you if I
was the original author of this check.

Sorry, I hope I didn't give the impression I thought you had done anything wrong here, because you definitely haven't. I appreciate all of your efforts (and am glad to see you back in the community), and my resigning from the review is not indicative of me not wanting to collaborate with you! It's more a matter of: I've got ~50 code reviews in my queue as of this morning, on top of my own work, and I didn't want you to have to ping this for months on end only to never get feedback from me because C++ Core Guideline reviews always stay at the bottom of my queue due to the amount of effort involved in most of them.

I treat the guidelines as just that: guidelines,
not rules. In the context of clang-tidy I think you're correct that some guidelines
are easily turned into usable diagnostics and a subset of those can become enforceable
rules with suggested fixits.

In this case, the check only issues diagnostics, not fixits. When the diagnostics result
in many false positives (as per the open bug), then I think it's reasonable to narrow the
scope of the check to omit the false positives.

This is not how clang-tidy typically handles coding guideline-specific rules. The general rule in clang-tidy is for checks based on coding guidelines to be configured to follow the guideline by default (with options to help tune things). If of sufficient value, we will add an alias check in bugprone (or another module that makes sense) and give it different default configuration settings than the guideline check, but users expect something that claims to check a guideline to actually check that guideline.

(The alternative is: we go back to the guideline authors and ask them to please improve their guidance and then we implement whatever the new rule says. But the end result is the same -- for checks in a module specific to a published set of guidelines, deviations from the spec are considered bugs to be fixed <somewhere>, and the guideline text is the final arbiter of what the correct behavior is.)

The worst thing a "guideline" checker can do is to constantly nag you about false
positives. This trains people to not run the checkers and/or ignore all their complaints.

We're in strong agreement that guideline checkers with untenable false positive rates are a very bad thing. I think this particular check is of insufficient quality to have been added in the first place because it's based on a set of rules that are not generally enforceable in a way that isn't a constantly nagging for reasonable real world code.

[LegalizeAdulthood]

In D116386#3227236, @aaron.ballman wrote:

We're in strong agreement that guideline checkers with untenable false positive rates are a
very bad thing. I think this particular check is of insufficient quality to have been added in
the first place because it's based on a set of rules that are not generally enforceable in a
way that isn't a constantly nagging for reasonable real world code.

I think this guideline is mostly what this check is trying to accomplish:
ES.31: Don't use macros for constants or "functions"

Now, for the first part about constants, the check was issuing way too many
false positives. That's what I fix in this review: it now only suggests that the
macro be replaced with a constexpr constant when the macro expansion is
truly a constant.

Ironically I was working on my own check that covers some aspects of
"Enum.1: Prefer enumerations over macros".
However, there isn't any way an automated tool could recognize the example
given in the guidelines because the macros in question don't share a common
prefix, which I've found is a good heuristic for enums disguised as macros.
There are additional heuristics one could apply, such as when a bunch of
constant macros are defined on successive lines.

[LegalizeAdulthood]

I'm adding the original author of this check as a reviewer.

[LegalizeAdulthood]

• clang-format
• use Token::isLiteral instead of homebrew token predicate
• rename predicate on token list to reflect isLiteral

[LegalizeAdulthood]

Ping.

This review has been waiting for a week without any comment.

[LegalizeAdulthood]

Ping again. This change isn't that long or complicated and fixes
a bug that results in false positives from this check.

Please give it a review.

[carlosgalvezp]

Code looks great! Would it be worth mentioning the change in the release notes? I also wonder if the check documentation needs some updates. From what i read in the discussion this rule has poor enforcement in the guidelines so perhaps it's good to clarify what exactly the check covers. I'm afk right now so i can't check this in detail :)

[carlosgalvezp]

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

83

LLVM coding standards say that this function should be "static", keeping anonymous namespaces only to classes

[LegalizeAdulthood]

In D116386#3252351, @carlosgalvezp wrote:

Would it be worth mentioning the change in the release notes? [...]

Updating the documentation based on the discussion thread and
the release notes are both good ideas, I'll do that and post an update.

[LegalizeAdulthood]

• For function isCapsOnly:
  • Declare as static per LLVM style guide
  • Simplify boolean expression
  • Use llvm::all_of on container
• Inline Function isLiteralTokenSequence as it is now simpler with llvm::all_of
• Update documentation:
  • Remove references to core guidelines that aren't implemented
  • Add mention of reduced false positives in the release notes
  • Give example in check documentation

[carlosgalvezp]

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

23–25

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)

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

11

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.

[LegalizeAdulthood]

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

11

It was always handled through an option on this check.
(Look at lines 49-56 of MacroUsageCheck.cpp)

It's a little bit odd, because it either checks for the names
or it checks for the constant/function like macros, it never
does both at the same time.

This is the way the check was originally written, I haven't
changed any of that.

[LegalizeAdulthood]

• Update from comments

[carlosgalvezp]

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

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

23–25

Nit: inline can be removed.

99

Nit: tab with spaces.

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

1

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?

[carlosgalvezp]

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

11

Ah I see it now! I got really confused by the CheckCapsOnly option. Not for this patch, but I think the following could be improved:

• Set it default to True, not False. People expect that check enforce a given guideline as good as possible by default. Options exist to deviate from the guidelines and relax them, which would be the case e.g. when introducing the check in an old codebase.

• Be renamed to something more descriptive and split it into 2 options with one single purpose. Right now this option controls a) enforcing ES.32 and b) applying warnings to macros with all caps or not.

[LegalizeAdulthood]

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

23–25

Yeah, my IDE flagged it but since you asked for the static .... :)

99

Not sure how that tab got in there, LOL.
Probably my IDE "helping" me in an unwanted fashion.

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

1

The u'' (UTF-16) and U'' (UTF-32) character literals were added in C++11.
The u8'' (UTF-8) character literal was added in C++17.
https://en.cppreference.com/w/cpp/language/character_literal

[LegalizeAdulthood]

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

11

Yeah, I wasn't a fan of the way this option was influencing the behavior of this check.

Can you open a github issue for the points you raised?

[carlosgalvezp]

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

23–25

Sorry for the confusion, I should have added inline fixes directly to make it clear :)

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

11

Absolutely, much better place for this!

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

1

Duh, I was checking string literal, not character literal
https://en.cppreference.com/w/cpp/language/string_literal

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

 //===--- MacroUsageCheck.cpp - clang-tidy----------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "MacroUsageCheck.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Lex/PPCallbacks.h"
 #include "clang/Lex/Preprocessor.h"
 #include "llvm/ADT/STLExtras.h"
 #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,
                     const MacroDirective *MD) override {
     if (SM.isWrittenInBuiltinFile(MD->getLocation()) ||
         MD->getMacroInfo()->isUsedForHeaderGuard() ||
         MD->getMacroInfo()->getNumTokens() == 0)
       return;

 void MacroUsageCheck::registerPPCallbacks(const SourceManager &SM,
                                           Preprocessor *PP,
                                           Preprocessor *ModuleExpanderPP) {
   PP->addPPCallbacks(std::make_unique<MacroUsageCallbacks>(
       this, SM, AllowedRegexp, CheckCapsOnly, IgnoreCommandLineMacros));
 }
 
 void MacroUsageCheck::warnMacro(const MacroDirective *MD, StringRef MacroName) {
-  StringRef 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.
-  if (MD->getMacroInfo()->isVariadic())
+  const MacroInfo *Info = MD->getMacroInfo();
+  StringRef Message;
+
+  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";
 
+  if (!Message.empty())
     diag(MD->getLocation(), Message) << MacroName;
 }
 
 void MacroUsageCheck::warnNaming(const MacroDirective *MD,
                                  StringRef MacroName) {
   diag(MD->getLocation(), "macro definition does not define the macro name "
                           "'%0' using all uppercase characters")
       << MacroName;
 }
 
 } // namespace cppcoreguidelines
 } // namespace tidy
 } // namespace clang

clang-tools-extra/docs/ReleaseNotes.rst

- Added support for globbing in `NOLINT*` expressions, to simplify suppressing
   multiple warnings in the same line.
 
 - Added support for `NOLINTBEGIN` ... `NOLINTEND` comments to suppress
   Clang-Tidy warnings over multiple lines.
 
 - 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
 ^^^^^^^^^^
 
 - New :doc:`bugprone-stringview-nullptr
   <clang-tidy/checks/bugprone-stringview-nullptr>` check.
 
   Checks for various ways that the ``const CharT*`` constructor of
   ``std::basic_string_view`` can be passed a null argument.

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

 .. title:: clang-tidy - cppcoreguidelines-macro-usage
 
 cppcoreguidelines-macro-usage
 =============================
 
 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
 -------
 
 .. option:: AllowedRegexp
 
     A regular expression to filter allowed macros. For example
     `DEBUG*|LIBTORRENT*|TORRENT*|UNI*` could be applied to filter `libtorrent`.

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

-// 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
 
 #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
 
 #define PROBLEMATIC_VARIADIC(...) (__VA_ARGS__)
 // 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 '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