[clang-tidy] Support parenthesized literals in modernize-macro-to-enum

[LegalizeAdulthood]

Summary

When scanning a macro expansion to examine it as a candidate enum,
first strip off arbitrary matching parentheses from the outside in,
then examine what remains to see if it is Lit, +Lit, -Lit or ~Lit.
If not, reject it as a possible enum candidate.

Fixes #54843

[njames93]

Would it not be better if these parens were stripped in the fixit as they are unnecessary in the enum value decl?

[LegalizeAdulthood]

In D123479#3442062, @njames93 wrote:

Would it not be better if these parens were stripped in the fixit as they are unnecessary in the enum value decl?

I prefer checks to do one thing only in a straightforward manner.

It's easier to place things before and after the macro expansion rather than modify the contents
of the expansion.

It doesn't feel like the responsibility of this check is to decide which parentheses are necessary or not.

[aaron.ballman]

In D123479#3442968, @LegalizeAdulthood wrote:

In D123479#3442062, @njames93 wrote:

Would it not be better if these parens were stripped in the fixit as they are unnecessary in the enum value decl?

To me, that would be ideal, but not strictly necessary (and perhaps doesn't scale super well across all fix-its) because the code is still correct with the parens, just a bit ugly. We have other similar situations with fix-its (like formatting).

I prefer checks to do one thing only in a straightforward manner.

It's easier to place things before and after the macro expansion rather than modify the contents
of the expansion.

It doesn't feel like the responsibility of this check is to decide which parentheses are necessary or not.

Hmm, I'm more on the fence. On the one hand, yes. On the other hand, there's no automatic cleanup to remove parentheses in clang-format (and there's no way I would trust clang-format if it added one, frankly). This check is suggesting a fix-it and a fix-it that keeps the parentheses means the user may feel compelled to go and manually change their code anyway, which largely removes the benefit of having a fix-it in the first place. That said, the fix-it produces correct code, and we could add a readability-redundant-parentheses check to clang-tidy if we cared deeply about redundant parens. We've had at least one attempt at that I could remember (and I hazily recall there may have been a second attempt), so there's some interest in that. That would solve the problem in a more general way than expecting each author of a fix-it to consider parens individually.

Regardless, this is incremental progress and it solves a real bug. LGTM aside from reflowing a comment.

clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp

351–354

Start at both ends and work towards the middle. This is clever, I like this!

clang-tools-extra/docs/clang-tidy/checks/modernize-macro-to-enum.rst

20–23

Reflowing the comment to 80 cols

[LegalizeAdulthood]

In D123479#3443401, @aaron.ballman wrote:

In D123479#3442968, @LegalizeAdulthood wrote:

In D123479#3442062, @njames93 wrote:

Would it not be better if these parens were stripped in the fixit as they are unnecessary in the enum value decl?

To me, that would be ideal, but not strictly necessary (and perhaps doesn't scale super well across all fix-its) because the code is still correct with the parens, just a bit ugly. We have other similar situations with fix-its (like formatting).

I prefer checks to do one thing only in a straightforward manner.

It's easier to place things before and after the macro expansion rather than modify the contents
of the expansion.

It doesn't feel like the responsibility of this check is to decide which parentheses are necessary or not.

Hmm, I'm more on the fence.

Me too. I am just not a fan of an automated check doing "other things" to my code
that aren't advertised in the description. This is a macro-to-enum check not a redundant-parens
check.

BTW, many IDEs highlight unnecessary parentheses and have "quick fixes" to remove them,
ReSharper for C++ is one such IDE add-on that does this.

I'd prefer there to be a readability-redundant-parentheses check that removes unnecessary
parentheses across the board everywhere, rather than it being a weird side-effect of turning
my macros into enums.

[LegalizeAdulthood]

Update from review comments

[LegalizeAdulthood]

• Update documentation to reflect multiple pairs of matching parens

[aaron.ballman]

In D123479#3443467, @LegalizeAdulthood wrote:

In D123479#3443401, @aaron.ballman wrote:

In D123479#3442968, @LegalizeAdulthood wrote:

In D123479#3442062, @njames93 wrote:

Would it not be better if these parens were stripped in the fixit as they are unnecessary in the enum value decl?

To me, that would be ideal, but not strictly necessary (and perhaps doesn't scale super well across all fix-its) because the code is still correct with the parens, just a bit ugly. We have other similar situations with fix-its (like formatting).

I prefer checks to do one thing only in a straightforward manner.

It's easier to place things before and after the macro expansion rather than modify the contents
of the expansion.

It doesn't feel like the responsibility of this check is to decide which parentheses are necessary or not.

Hmm, I'm more on the fence.

Me too. I am just not a fan of an automated check doing "other things" to my code
that aren't advertised in the description. This is a macro-to-enum check not a redundant-parens
check.

What puts me on the fence here is -- your check adds the fix-it and it knows that the parens are unnecessary because there can be no order of operations or side effect issues. So yes, you're not writing a redundant parens check; you're writing a macro to enum check that proposes fixes that keep redundant parens when it could be argued that it's reasonable to remove them by not copying them into the fix-it in the first place.

BTW, many IDEs highlight unnecessary parentheses and have "quick fixes" to remove them,
ReSharper for C++ is one such IDE add-on that does this.

I'd prefer there to be a readability-redundant-parentheses check that removes unnecessary
parentheses across the board everywhere, rather than it being a weird side-effect of turning
my macros into enums.

I tend to come to this conclusion as well. The fix-it is syntactically valid and semantically correct, so it's not wrong to leave it the way it is.

clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp

void MacroToEnumCallbacks::MacroDefined(const Token &MacroNameTok,
 
   if (insideConditional())
     return;
 
   if (SM.getFilename(MD->getLocation()).empty())
     return;
 
   const MacroInfo *Info = MD->getMacroInfo();
-  if (Info->isFunctionLike() || Info->isBuiltinMacro() ||
-      Info->tokens().empty() || Info->tokens().size() > 2)
+  ArrayRef<Token> MacroTokens = Info->tokens();
+  if (Info->isFunctionLike() || Info->isBuiltinMacro() || MacroTokens.empty())
     return;
 
-  // It can be +Lit, -Lit or just Lit.
-  Token Tok = Info->tokens().front();
-  if (Info->tokens().size() == 2) {
-    if (!Tok.isOneOf(tok::TokenKind::minus, tok::TokenKind::plus,
-                     tok::TokenKind::tilde))
+  // Return Lit when +Lit, -Lit or ~Lit; otherwise return Unknown.
+  Token Unknown;
+  Unknown.setKind(tok::TokenKind::unknown);
+  auto GetUnopArg = [Unknown](Token First, Token Second) {
+    return First.isOneOf(tok::TokenKind::minus, tok::TokenKind::plus,
+                         tok::TokenKind::tilde)
+               ? Second
+               : Unknown;
+  };
+
+  // It could just be a single token.
+  Token Tok = MacroTokens.front();
+
+  // It can be any arbitrary nesting of matched parentheses around
+  // +Lit, -Lit, ~Lit or Lit.
+  if (MacroTokens.size() > 2) {
+    // Strip off matching '(', ..., ')' token pairs.
+    size_t Begin = 0;
+    size_t End = MacroTokens.size() - 1;
+    assert(End >= 2U);
+    for (; Begin < MacroTokens.size() / 2; ++Begin, --End) {
+      if (!MacroTokens[Begin].is(tok::TokenKind::l_paren) ||
+          !MacroTokens[End].is(tok::TokenKind::r_paren))
+        break;
+    }
+    size_t Size = End >= Begin ? (End - Begin + 1U) : 0U;
+
+    // It was a single token inside matching parens.
+    if (Size == 1)
+      Tok = MacroTokens[Begin];
+    else if (Size == 2)
+      // It can be +Lit, -Lit or ~Lit.
+      Tok = GetUnopArg(MacroTokens[Begin], MacroTokens[End]);
+    else
+      // Zero or too many tokens after we stripped matching parens.
       return;
-    Tok = Info->tokens().back();
+  } else if (MacroTokens.size() == 2) {
+    // It can be +Lit, -Lit, or ~Lit.
+    Tok = GetUnopArg(MacroTokens.front(), MacroTokens.back());
   }
+
   if (!Tok.isLiteral() || isStringLiteral(Tok.getKind()) ||
       !isIntegralConstant(Tok))
     return;
 
   if (!isConsecutiveMacro(MD))
     newEnum();
   Enums.back().emplace_back(MacroNameTok, MD);
   rememberLastMacroLocation(MD);

clang-tools-extra/docs/clang-tidy/checks/modernize-macro-to-enum.rst

 .. title:: clang-tidy - modernize-macro-to-enum
 
 modernize-macro-to-enum
 =======================
 
 Replaces groups of adjacent macros with an unscoped anonymous enum.
 Using an unscoped anonymous enum ensures that everywhere the macro
 token was used previously, the enumerator name may be safely used.
 
 This check can be used to enforce the C++ core guideline `Enum.1:
 Prefer enumerations over macros
 <https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#enum1-prefer-enumerations-over-macros>`_,
 within the constraints outlined below.
 
 Potential macros for replacement must meet the following constraints:
 
-- Macros must expand only to integral literal tokens.  The unary operators
-  plus, minus and tilde are recognized to allow for positive, negative
-  and bitwise negated integers.  More complicated integral constant
-  expressions are not currently recognized by this check.
+- Macros must expand only to integral literal tokens or simple expressions
+  of literal tokens.  The unary operators plus, minus and tilde are
+  recognized to allow for positive, negative and bitwise negated integers.
+  The above expressions may also be surrounded by matching pairs of
+  parentheses.  More complicated integral constant expressions are not
+  recognized by this check.
 - Macros must be defined on sequential source file lines, or with
   only comment lines in between macro definitions.
 - Macros must all be defined in the same source file.
 - Macros must not be defined within a conditional compilation block.
   (Conditional include guards are exempt from this constraint.)
 - Macros must not be defined adjacent to other preprocessor directives.
 - Macros must not be used in any conditional preprocessing directive.
 - Macros must not be undefined.
 
 Each cluster of macros meeting the above constraints is presumed to
 be a set of values suitable for replacement by an anonymous enum.
 From there, a developer can give the anonymous enum a name and
 continue refactoring to a scoped enum if desired.  Comments on the
 same line as a macro definition or between subsequent macro definitions
 are preserved in the output.  No formatting is assumed in the provided
 replacements, although clang-tidy can optionally format all fixes.
 
 Examples:
 
 .. code-block:: c++
 
   #define RED   0xFF0000
   #define GREEN 0x00FF00
   #define BLUE  0x0000FF
 
+  #define TM_NONE (-1) // No method selected.
   #define TM_ONE 1    // Use tailored method one.
   #define TM_TWO 2    // Use tailored method two.  Method two
                       // is preferable to method one.
   #define TM_THREE 3  // Use tailored method three.
 
 becomes
 
 .. code-block:: c++
 
   enum {
   RED = 0xFF0000,
   GREEN = 0x00FF00,
   BLUE = 0x0000FF
   };
 
   enum {
+  TM_NONE = (-1), // No method selected.
   TM_ONE = 1,    // Use tailored method one.
   TM_TWO = 2,    // Use tailored method two.  Method two
                       // is preferable to method one.
   TM_THREE = 3  // Use tailored method three.
   };

clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp

 // CHECK-FIXES-NEXT: BASE3 = 0b0011,  // binary
 // CHECK-FIXES-NEXT: SUFFIX1 = +1U,
 // CHECK-FIXES-NEXT: SUFFIX2 = -1L,
 // CHECK-FIXES-NEXT: SUFFIX3 = +1UL,
 // CHECK-FIXES-NEXT: SUFFIX4 = -1LL,
 // CHECK-FIXES-NEXT: SUFFIX5 = +1ULL
 // CHECK-FIXES-NEXT: };
 
+// A limited form of constant expression is recognized: a parenthesized
+// literal or a parenthesized literal with the unary operators +, - or ~.
+#define PAREN1 (-1)
+#define PAREN2 (1)
+#define PAREN3 (+1)
+#define PAREN4 (~1)
+// CHECK-MESSAGES: :[[@LINE-4]]:1: warning: replace macro with enum
+// CHECK-MESSAGES: :[[@LINE-5]]:9: warning: macro 'PAREN1' defines an integral constant; prefer an enum instead
+// CHECK-MESSAGES: :[[@LINE-5]]:9: warning: macro 'PAREN2' defines an integral constant; prefer an enum instead
+// CHECK-MESSAGES: :[[@LINE-5]]:9: warning: macro 'PAREN3' defines an integral constant; prefer an enum instead
+// CHECK-MESSAGES: :[[@LINE-5]]:9: warning: macro 'PAREN4' defines an integral constant; prefer an enum instead
+// CHECK-FIXES: enum {
+// CHECK-FIXES-NEXT: PAREN1 = (-1),
+// CHECK-FIXES-NEXT: PAREN2 = (1),
+// CHECK-FIXES-NEXT: PAREN3 = (+1),
+// CHECK-FIXES-NEXT: PAREN4 = (~1)
+// CHECK-FIXES-NEXT: };
+
+// More complicated parenthesized expressions are excluded.
+// Expansions that are not surrounded by parentheses are excluded.
+// Nested matching parentheses are stripped.
+#define COMPLEX_PAREN1 (x+1)
+#define COMPLEX_PAREN2 (x+1
+#define COMPLEX_PAREN3 (())
+#define COMPLEX_PAREN4 ()
+#define COMPLEX_PAREN5 (+1)
+#define COMPLEX_PAREN6 ((+1))
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: replace macro with enum
+// CHECK-MESSAGES: :[[@LINE-3]]:9: warning: macro 'COMPLEX_PAREN5' defines an integral constant; prefer an enum instead
+// CHECK-MESSAGES: :[[@LINE-3]]:9: warning: macro 'COMPLEX_PAREN6' defines an integral constant; prefer an enum instead
+// CHECK-FIXES: enum {
+// CHECK-FIXES-NEXT: COMPLEX_PAREN5 = (+1),
+// CHECK-FIXES-NEXT: COMPLEX_PAREN6 = ((+1))
+// CHECK-FIXES-NEXT: };
+
 // Macros appearing in conditional expressions can't be replaced
 // by enums.
 #define USE_FOO 1
 #define USE_BAR 0
 #define USE_IF 1
 #define USE_ELIF 1
 #define USE_IFDEF 1
 #define USE_IFNDEF 1