[clang-tidy] fix false-negative for macros in readability-math-missing-parentheses

[5chmidti]

When a binary operator is the last operand of a macro, the end location
that is past the BinaryOperator will be inside the macro and therefore an
invalid location to insert a FixIt into, which is why the check bails
when encountering such a pattern.
However, the end location is only required for the FixIt and the
diagnostic can still be emitted, just without an attached fix.

[llvmbot]

@llvm/pr-subscribers-clang-tidy

@llvm/pr-subscribers-clang-tools-extra

Author: Julian Schmidt (5chmidti)

Changes

When a binary operator is the last operand of a macro, the end location
that is past the BinaryOperator will be inside the macro and therefore an
invalid location to insert a FixIt into, which is why the check bails
when encountering such a pattern.
However, the end location is only required for the FixIt and the
diagnostic can still be emitted, just without an attached fix.

---

Full diff: https://github.com/llvm/llvm-project/pull/90279.diff

2 Files Affected:

• (modified) clang-tools-extra/clang-tidy/readability/MathMissingParenthesesCheck.cpp (+9-7)
• (modified) clang-tools-extra/test/clang-tidy/checkers/readability/math-missing-parentheses.cpp (+22)

diff --git a/clang-tools-extra/clang-tidy/readability/MathMissingParenthesesCheck.cpp b/clang-tools-extra/clang-tidy/readability/MathMissingParenthesesCheck.cpp
index d1e20b9074cec1..65fd296094915b 100644
--- a/clang-tools-extra/clang-tidy/readability/MathMissingParenthesesCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/MathMissingParenthesesCheck.cpp
@@ -61,19 +61,21 @@ static void addParantheses(const BinaryOperator *BinOp,
     const clang::SourceLocation StartLoc = BinOp->getBeginLoc();
     const clang::SourceLocation EndLoc =
         clang::Lexer::getLocForEndOfToken(BinOp->getEndLoc(), 0, SM, LangOpts);
-    if (EndLoc.isInvalid())
-      return;
 
-    Check->diag(StartLoc,
-                "'%0' has higher precedence than '%1'; add parentheses to "
-                "explicitly specify the order of operations")
+    auto Diag =
+        Check->diag(StartLoc,
+                    "'%0' has higher precedence than '%1'; add parentheses to "
+                    "explicitly specify the order of operations")
         << (Precedence1 > Precedence2 ? BinOp->getOpcodeStr()
                                       : ParentBinOp->getOpcodeStr())
         << (Precedence1 > Precedence2 ? ParentBinOp->getOpcodeStr()
                                       : BinOp->getOpcodeStr())
-        << FixItHint::CreateInsertion(StartLoc, "(")
-        << FixItHint::CreateInsertion(EndLoc, ")")
         << SourceRange(StartLoc, EndLoc);
+
+    if (EndLoc.isValid()) {
+      Diag << FixItHint::CreateInsertion(StartLoc, "(")
+           << FixItHint::CreateInsertion(EndLoc, ")");
+    }
   }
 
   addParantheses(dyn_cast<BinaryOperator>(BinOp->getLHS()->IgnoreImpCasts()),
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/math-missing-parentheses.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/math-missing-parentheses.cpp
index edbe2e1c37c770..a6045c079a4823 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/readability/math-missing-parentheses.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability/math-missing-parentheses.cpp
@@ -16,6 +16,13 @@ int bar(){
     return 4;
 }
 
+int sink(int);
+#define FUN(ARG) (sink(ARG))
+#define FUN2(ARG) sink((ARG))
+#define FUN3(ARG) sink(ARG)
+#define FUN4(ARG) sink(1 + ARG)
+#define FUN5(ARG) sink(4 * ARG)
+
 class fun{
 public:
     int A;
@@ -117,4 +124,19 @@ void f(){
     //CHECK-MESSAGES: :[[@LINE+2]]:94: warning: '/' has higher precedence than '-'; add parentheses to explicitly specify the order of operations [readability-math-missing-parentheses]
     //CHECK-FIXES: int q = (1 MACRO_ADD (2 MACRO_MULTIPLY 3)) MACRO_OR ((4 MACRO_AND 5) MACRO_XOR (6 MACRO_SUBTRACT (7 MACRO_DIVIDE 8)));
     int q = 1 MACRO_ADD 2 MACRO_MULTIPLY 3 MACRO_OR 4 MACRO_AND 5 MACRO_XOR 6 MACRO_SUBTRACT 7 MACRO_DIVIDE 8; // No warning
+
+    //CHECK-MESSAGES: :[[@LINE+1]]:21: warning: '*' has higher precedence than '+'; add parentheses to explicitly specify the order of operations [readability-math-missing-parentheses]
+    int r = FUN(0 + 1 * 2);
+
+    //CHECK-MESSAGES: :[[@LINE+1]]:22: warning: '*' has higher precedence than '+'; add parentheses to explicitly specify the order of operations [readability-math-missing-parentheses]
+    int s = FUN2(0 + 1 * 2);
+
+    //CHECK-MESSAGES: :[[@LINE+1]]:22: warning: '*' has higher precedence than '+'; add parentheses to explicitly specify the order of operations [readability-math-missing-parentheses]
+    int t = FUN3(0 + 1 * 2);
+
+    //CHECK-MESSAGES: :[[@LINE+1]]:18: warning: '*' has higher precedence than '+'; add parentheses to explicitly specify the order of operations [readability-math-missing-parentheses]
+    int u = FUN4(1 * 2);
+
+    //CHECK-MESSAGES: :[[@LINE+1]]:13: warning: '*' has higher precedence than '+'; add parentheses to explicitly specify the order of operations [readability-math-missing-parentheses]
+    int v = FUN5(0 + 1);
 }

[5chmidti]

CC @11happy (couldn't add you as a reviewer)

[11happy]

LGTM.
Thank you for CCing me.

[PiotrZSL]

LGTM.

[HerrCai0907]

LGTM