[clang-tools-extra] 32d8823 - [clang-tidy] Tweak diagnostics for bugprone-assign-in-if-condition

[Nathan James via cfe-commits]

Author: Nathan James
Date: 2022-08-28T00:21:40+01:00
New Revision: 32d88239ae654239f16b516ee81ee9ff88b0ce07

URL: https://github.com/llvm/llvm-project/commit/32d88239ae654239f16b516ee81ee9ff88b0ce07
DIFF: https://github.com/llvm/llvm-project/commit/32d88239ae654239f16b516ee81ee9ff88b0ce07.diff

LOG: [clang-tidy] Tweak diagnostics for bugprone-assign-in-if-condition

Currently the diagnostic is printed at the start of the assignment expression, This can be misleading.
Having the location for the diagnostic be the location of the assignment operator is much more intuitive.

Reviewed By: gribozavr2

Differential Revision: https://reviews.llvm.org/D132795

Added: 
    

Modified: 
    clang-tools-extra/clang-tidy/bugprone/AssignmentInIfConditionCheck.cpp
    clang-tools-extra/clang-tidy/bugprone/AssignmentInIfConditionCheck.h
    clang-tools-extra/test/clang-tidy/checkers/bugprone/assignment-in-if-condition.cpp

Removed: 
    


################################################################################
diff  --git a/clang-tools-extra/clang-tidy/bugprone/AssignmentInIfConditionCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/AssignmentInIfConditionCheck.cpp
index 05f0ae74282e..47a82e3c09e2 100644
--- a/clang-tools-extra/clang-tidy/bugprone/AssignmentInIfConditionCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/AssignmentInIfConditionCheck.cpp
@@ -61,14 +61,18 @@ void AssignmentInIfConditionCheck::check(
   Visitor(*this).TraverseAST(*Result.Context);
 }
 
-void AssignmentInIfConditionCheck::report(const Expr *MatchedDecl) {
-  diag(MatchedDecl->getBeginLoc(),
-       "an assignment within an 'if' condition is bug-prone");
-  diag(MatchedDecl->getBeginLoc(),
+void AssignmentInIfConditionCheck::report(const Expr *AssignmentExpr) {
+  SourceLocation OpLoc =
+      isa<BinaryOperator>(AssignmentExpr)
+          ? cast<BinaryOperator>(AssignmentExpr)->getOperatorLoc()
+          : cast<CXXOperatorCallExpr>(AssignmentExpr)->getOperatorLoc();
+
+  diag(OpLoc, "an assignment within an 'if' condition is bug-prone")
+      << AssignmentExpr->getSourceRange();
+  diag(OpLoc,
        "if it should be an assignment, move it out of the 'if' condition",
        DiagnosticIDs::Note);
-  diag(MatchedDecl->getBeginLoc(),
-       "if it is meant to be an equality check, change '=' to '=='",
+  diag(OpLoc, "if it is meant to be an equality check, change '=' to '=='",
        DiagnosticIDs::Note);
 }
 

diff  --git a/clang-tools-extra/clang-tidy/bugprone/AssignmentInIfConditionCheck.h b/clang-tools-extra/clang-tidy/bugprone/AssignmentInIfConditionCheck.h
index f84ae93ed2eb..f49dda24c9a9 100644
--- a/clang-tools-extra/clang-tidy/bugprone/AssignmentInIfConditionCheck.h
+++ b/clang-tools-extra/clang-tidy/bugprone/AssignmentInIfConditionCheck.h
@@ -25,7 +25,7 @@ class AssignmentInIfConditionCheck : public ClangTidyCheck {
       : ClangTidyCheck(Name, Context) {}
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
-  void report(const Expr *E);
+  void report(const Expr *AssignmentExpr);
 };
 
 } // namespace bugprone

diff  --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/assignment-in-if-condition.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/assignment-in-if-condition.cpp
index 20c30d73516b..ad0d360d99fe 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/assignment-in-if-condition.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/assignment-in-if-condition.cpp
@@ -3,7 +3,7 @@
 void f(int arg) {
   int f = 3;
   if ((f = arg) || (f == (arg + 1)))
-  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: an assignment within an 'if' condition is bug-prone [bugprone-assignment-in-if-condition]
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: an assignment within an 'if' condition is bug-prone [bugprone-assignment-in-if-condition]
   {
     f = 5;
   }
@@ -12,7 +12,7 @@ void f(int arg) {
 void f1(int arg) {
   int f = 3;
   if ((f == arg) || (f = (arg + 1)))
-  // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: an assignment within an 'if' condition is bug-prone [bugprone-assignment-in-if-condition]
+  // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: an assignment within an 'if' condition is bug-prone [bugprone-assignment-in-if-condition]
   {
     f = 5;
   }
@@ -21,7 +21,7 @@ void f1(int arg) {
 void f2(int arg) {
   int f = 3;
   if (f = arg)
-  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: an assignment within an 'if' condition is bug-prone [bugprone-assignment-in-if-condition]
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: an assignment within an 'if' condition is bug-prone [bugprone-assignment-in-if-condition]
   {
     f = 5;
   }
@@ -32,7 +32,7 @@ volatile int v = 32;
 void f3(int arg) {
   int f = 3;
   if ((f == arg) || ((arg + 6 < f) && (f = v)))
-  // CHECK-MESSAGES: :[[@LINE-1]]:40: warning: an assignment within an 'if' condition is bug-prone [bugprone-assignment-in-if-condition]
+  // CHECK-MESSAGES: :[[@LINE-1]]:42: warning: an assignment within an 'if' condition is bug-prone [bugprone-assignment-in-if-condition]
   {
     f = 5;
   }
@@ -41,11 +41,11 @@ void f3(int arg) {
 void f4(int arg) {
   int f = 3;
   if ((f == arg) || ((arg + 6 < f) && ((f = v) || (f < 8))))
-  // CHECK-MESSAGES: :[[@LINE-1]]:41: warning: an assignment within an 'if' condition is bug-prone [bugprone-assignment-in-if-condition]
+  // CHECK-MESSAGES: :[[@LINE-1]]:43: warning: an assignment within an 'if' condition is bug-prone [bugprone-assignment-in-if-condition]
   {
     f = 5;
   } else if ((arg + 8 < f) && ((f = v) || (f < 8)))
-  // CHECK-MESSAGES: :[[@LINE-1]]:33: warning: an assignment within an 'if' condition is bug-prone [bugprone-assignment-in-if-condition]
+  // CHECK-MESSAGES: :[[@LINE-1]]:35: warning: an assignment within an 'if' condition is bug-prone [bugprone-assignment-in-if-condition]
   {
     f = 6;
   }
@@ -68,12 +68,12 @@ void f5(int arg) {
     f = 6;
   }
   if (bo = 3)
-  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: an assignment within an 'if' condition is bug-prone [bugprone-assignment-in-if-condition]
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: an assignment within an 'if' condition is bug-prone [bugprone-assignment-in-if-condition]
   {
     f = 7;
   }
   if ((arg == 3) || (bo = 6))
-  // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: an assignment within an 'if' condition is bug-prone [bugprone-assignment-in-if-condition]
+  // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: an assignment within an 'if' condition is bug-prone [bugprone-assignment-in-if-condition]
   {
     f = 8;
   }