[clang-tidy] misc-macro-parentheses: Don't insert parentheses in variable declarations. Fixes bugzilla 26273

[danielmarjamaki]

Summary

This is a quick fix for bugzilla 26273. parentheses should not be inserted in variable declarations.

This patch will introduce false negatives when a macro looks like it could be variable declaration but is not.

For example:

#define ABC   A*B*C

To avoid false positives in such expression, I am guessing that non-keywords such as "A", "B" and "C" are types or qualifiers so "A*B*C" is some variable declaration. My primary focus was to avoid FP.

Maybe there is a method that I am not aware of, to see if there is a type "A" during preprocessing.. that would be great.

The "possibleVarDecl" could be much more clever. For instance, variable declarations can at least contain :: < & also, I could handle those better also but that would mean more false negatives.

[danielmarjamaki]

The "possibleVarDecl" could be much more clever. For instance, variable declarations can at least contain :: < & also, I could handle those better also but that would mean more false negatives.

I now saw bugzilla 27399 also.

I do need to handle templates also.

so I will send a new patch with updated possibleVarDecl().

[danielmarjamaki]

Updated diff. Now also handles bugzilla 27399.

[alexfh]

Looks good with a few style comments.

clang-tidy/misc/MacroParenthesesCheck.cpp

82 ↗

(On Diff #59208)

Either If we see int/short/struct/etc., or If we see int, short, struct, etc., .
Please also add a trailing period.

86 ↗

(On Diff #59208)

Trailing period, please.

93 ↗

(On Diff #59208)

Please remove parentheses around the method call.

97 ↗

(On Diff #59208)

Trailing period.

105 ↗

(On Diff #59208)

Trailing period.

158 ↗

(On Diff #59208)

ditto

clang-tools-extra/trunk/clang-tidy/misc/MacroParenthesesCheck.cpp

 
 namespace clang {
 namespace tidy {
 namespace misc {
 
 namespace {
 class MacroParenthesesPPCallbacks : public PPCallbacks {
 public:
-  MacroParenthesesPPCallbacks(Preprocessor *PP,
-                              MacroParenthesesCheck *Check)
+  MacroParenthesesPPCallbacks(Preprocessor *PP, MacroParenthesesCheck *Check)
       : PP(PP), Check(Check) {}
 
   void MacroDefined(const Token &MacroNameTok,
                     const MacroDirective *MD) override {
     replacementList(MacroNameTok, MD->getMacroInfo());
     argument(MacroNameTok, MD->getMacroInfo());
   }
 

 /// Warning is written when one of these operators are not within parentheses.
 static bool isWarnOp(const Token &T) {
   // FIXME: This is an initial list of operators. It can be tweaked later to
   // get more positives or perhaps avoid some false positive.
   return T.isOneOf(tok::plus, tok::minus, tok::star, tok::slash, tok::percent,
                    tok::amp, tok::pipe, tok::caret);
 }
 
+/// Is given Token a keyword that is used in variable declarations?
+static bool isVarDeclKeyword(const Token &T) {
+  return T.isOneOf(tok::kw_bool, tok::kw_char, tok::kw_short, tok::kw_int,
+                   tok::kw_long, tok::kw_float, tok::kw_double, tok::kw_const,
+                   tok::kw_enum, tok::kw_inline, tok::kw_static, tok::kw_struct,
+                   tok::kw_signed, tok::kw_unsigned);
+}
+
+/// Is there a possible variable declaration at Tok?
+static bool possibleVarDecl(const MacroInfo *MI, const Token *Tok) {
+  if (Tok == MI->tokens_end())
+    return false;
+
+  // If we see int/short/struct/etc., just assume this is a variable declaration.
+  if (isVarDeclKeyword(*Tok))
+    return true;
+
+  // Variable declarations start with identifier or coloncolon.
+  if (!Tok->isOneOf(tok::identifier, tok::raw_identifier, tok::coloncolon))
+    return false;
+
+  // Skip possible types, etc
+  while (
+      Tok != MI->tokens_end() &&
+      Tok->isOneOf(tok::identifier, tok::raw_identifier, tok::coloncolon,
+                    tok::star, tok::amp, tok::ampamp, tok::less, tok::greater))
+    Tok++;
+
+  // Return true for possible variable declarations.
+  return Tok == MI->tokens_end() ||
+         Tok->isOneOf(tok::equal, tok::semi, tok::l_square, tok::l_paren) ||
+         isVarDeclKeyword(*Tok);
+}
+
 void MacroParenthesesPPCallbacks::replacementList(const Token &MacroNameTok,
                                                   const MacroInfo *MI) {
+  // Make sure macro replacement isn't a variable declaration.
+  if (possibleVarDecl(MI, MI->tokens_begin()))
+    return;
+
   // Count how deep we are in parentheses/braces/squares.
   int Count = 0;
 
   // SourceLocation for error
   SourceLocation Loc;
 
   for (auto TI = MI->tokens_begin(), TE = MI->tokens_end(); TI != TE; ++TI) {
     const Token &Tok = *TI;
Check->diag(Loc, "macro replacement list should be enclosed in parentheses")
         << FixItHint::CreateInsertion(Last.getLocation().getLocWithOffset(
                                           PP->getSpelling(Last).length()),
                                       ")");
   }
 }
 
 void MacroParenthesesPPCallbacks::argument(const Token &MacroNameTok,
                                            const MacroInfo *MI) {
   
+  // Skip variable declaration.
+  bool VarDecl = possibleVarDecl(MI, MI->tokens_begin());
+
   for (auto TI = MI->tokens_begin(), TE = MI->tokens_end(); TI != TE; ++TI) {
     // First token.
     if (TI == MI->tokens_begin())
       continue;
 
     // Last token.
     if ((TI + 1) == MI->tokens_end())
       continue;
 
     const Token &Prev = *(TI - 1);
     const Token &Next = *(TI + 1);
 
     const Token &Tok = *TI;
 
+    // There should not be extra parentheses in possible variable declaration.
+    if (VarDecl) {
+      if (Tok.isOneOf(tok::equal, tok::semi, tok::l_square, tok::l_paren))
+        VarDecl = false;
+      continue;
+    }
+
     // Only interested in identifiers.
     if (!Tok.isOneOf(tok::identifier, tok::raw_identifier))
       continue;
 
     // Only interested in macro arguments.
     if (MI->getArgumentNum(Tok.getIdentifierInfo()) < 0)
       continue;
 

clang-tools-extra/trunk/test/clang-tidy/misc-macro-parentheses.cpp

 // RUN: %check_clang_tidy %s misc-macro-parentheses %t
 
 #define BAD1              -1
 // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: macro replacement list should be enclosed in parentheses [misc-macro-parentheses]
 #define BAD2              1+2
 // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: macro replacement list should be enclosed in parentheses [misc-macro-parentheses]
 #define BAD3(A)           (A+1)
 // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: macro argument should be enclosed in parentheses [misc-macro-parentheses]
 #define BAD4(x)           ((unsigned char)(x & 0xff))
 // CHECK-MESSAGES: :[[@LINE-1]]:44: warning: macro argument should be enclosed in parentheses [misc-macro-parentheses]
+#define BAD5(X)           A*B=(C*)X+2
+// CHECK-MESSAGES: :[[@LINE-1]]:35: warning: macro argument should be enclosed in parentheses [misc-macro-parentheses]
 
 #define GOOD1             1
 #define GOOD2             (1+2)
 #define GOOD3(A)          #A
 #define GOOD4(A,B)        A ## B
 #define GOOD5(T)          ((T*)0)
 #define GOOD6(B)          "A" B "C"
 #define GOOD7(b)          A b

 #define GOOD23(type)      (type::Field)
 #define GOOD24(t)         std::set<t> s
 #define GOOD25(t)         std::set<t,t,t> s
 #define GOOD26(x)         (a->*x)
 #define GOOD27(x)         (a.*x)
 #define GOOD28(x)         namespace x {int b;}
 #define GOOD29(...)       std::cout << __VA_ARGS__;
 #define GOOD30(args...)   std::cout << args;
+#define GOOD31(X)         A*X=2
+#define GOOD32(X)         std::vector<X>
 
 // These are allowed for now..
 #define MAYBE1            *12.34
 #define MAYBE2            <<3