https://github.com/mark-zilber updated https://github.com/llvm/llvm-project/pull/231704
>From 4e46f68dad8e4f1d346582702a73db1bb7c0652c Mon Sep 17 00:00:00 2001 From: Mark Zilber <[email protected]> Date: Wed, 14 Oct 2026 03:06:57 +0400 Subject: [PATCH 1/2] [clang-tidy] Fix assignment-in-if-condition false positive in statement expressions The check traversed into GNU statement expressions and diagnosed assignments in separate statements inside them, e.g. `({ ...; r = ...; r; })`. Now the check only looks at the last statement of a GNU statement expression, skipping any labels or attributes on it. An assignment that is itself the value is still diagnosed. Fixes #210638 --- .../bugprone/AssignmentInIfConditionCheck.cpp | 17 +++++++ clang-tools-extra/docs/ReleaseNotes.md | 5 +++ .../bugprone/assignment-in-if-condition.cpp | 44 +++++++++++++++++++ 3 files changed, 66 insertions(+) diff --git a/clang-tools-extra/clang-tidy/bugprone/AssignmentInIfConditionCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/AssignmentInIfConditionCheck.cpp index d5d8a29d969693..748b026fb2b81a 100644 --- a/clang-tools-extra/clang-tidy/bugprone/AssignmentInIfConditionCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/AssignmentInIfConditionCheck.cpp @@ -45,6 +45,23 @@ void AssignmentInIfConditionCheck::check( return true; } + // Only traverse the result expression of a statement expression, + // looking through labels and attributes on the last statement. + bool TraverseStmtExpr(StmtExpr *SE, + DataRecursionQueue *Queue = nullptr) { + Stmt *Last = SE->getSubStmt()->body_back(); + while (Last && !isa<Expr>(Last)) + if (auto *LS = dyn_cast<LabelStmt>(Last)) + Last = LS->getSubStmt(); + else if (auto *AS = dyn_cast<AttributedStmt>(Last)) + Last = AS->getSubStmt(); + else + return true; + if (auto *E = dyn_cast_if_present<Expr>(Last)) + return TraverseStmt(E, Queue); + return true; + } + bool VisitBinaryOperator(BinaryOperator *BO) { if (BO->isAssignmentOp()) Check.report(BO); diff --git a/clang-tools-extra/docs/ReleaseNotes.md b/clang-tools-extra/docs/ReleaseNotes.md index 14480052535567..762cdf9aa9fe34 100644 --- a/clang-tools-extra/docs/ReleaseNotes.md +++ b/clang-tools-extra/docs/ReleaseNotes.md @@ -174,6 +174,11 @@ infrastructure are described first, followed by tool-specific sections. #### Changes in existing checks +- Improved {doc}`bugprone-assignment-in-if-condition + <clang-tidy/checks/bugprone/assignment-in-if-condition>` check by ignoring + assignments inside GNU statement expressions unless they form the value of + the statement expression. + - Improved {doc}`bugprone-easily-swappable-parameters <clang-tidy/checks/bugprone/easily-swappable-parameters>` check by no longer emitting empty notes for type aliases when {option}`ModelImplicitConversions` 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 ad0d360d99fefc..e62b16540f4901 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 @@ -80,6 +80,33 @@ void f5(int arg) { v = f; } +void f6(int arg) { + int x; + if (({ x = 5; })) + // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: an assignment within an 'if' condition is bug-prone [bugprone-assignment-in-if-condition] + return; +} + +void f7(int arg) { + if (({ int r; r = arg; (r = 1); })) + // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: an assignment within an 'if' condition is bug-prone [bugprone-assignment-in-if-condition] + return; +} + +void f8(int arg) { + if (({ label: arg = 1; })) + // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: an assignment within an 'if' condition is bug-prone [bugprone-assignment-in-if-condition] + return; +} + +int get_value(); + +void f9(int arg) { + if (({ [[clang::nomerge]] arg = get_value(); })) + // CHECK-MESSAGES: :[[@LINE-1]]:33: warning: an assignment within an 'if' condition is bug-prone [bugprone-assignment-in-if-condition] + return; +} + // Tests that shouldn't trigger warnings. void awesome_f2(int arg) { int f = 3; @@ -102,6 +129,23 @@ void awesome_f4(int arg) { } } +#define STMT_EXPR_MACRO(arg) \ + ({ \ + bool R; \ + R = (arg) != 1; \ + R; \ + }) + +void awesome_f5(int arg) { + if (STMT_EXPR_MACRO(arg)) + return; +} + +void awesome_f6(int arg) { + if (({ int r; r = arg; r; })) + return; +} + template <typename Func> bool exec(Func F) { return F(); } void lambda_if() { >From cfaf40d8fa15fea06fe4092836403e43a0adf914 Mon Sep 17 00:00:00 2001 From: Mark Zilber <[email protected]> Date: Thu, 15 Oct 2026 10:54:09 +0400 Subject: [PATCH 2/2] Use ValueStmt::getExprStmt and add nested/operator= tests --- .../bugprone/AssignmentInIfConditionCheck.cpp | 13 ++----- .../bugprone/assignment-in-if-condition.cpp | 37 +++++++++++++++++-- 2 files changed, 36 insertions(+), 14 deletions(-) diff --git a/clang-tools-extra/clang-tidy/bugprone/AssignmentInIfConditionCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/AssignmentInIfConditionCheck.cpp index 748b026fb2b81a..ac5ab07fd3e061 100644 --- a/clang-tools-extra/clang-tidy/bugprone/AssignmentInIfConditionCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/AssignmentInIfConditionCheck.cpp @@ -49,16 +49,9 @@ void AssignmentInIfConditionCheck::check( // looking through labels and attributes on the last statement. bool TraverseStmtExpr(StmtExpr *SE, DataRecursionQueue *Queue = nullptr) { - Stmt *Last = SE->getSubStmt()->body_back(); - while (Last && !isa<Expr>(Last)) - if (auto *LS = dyn_cast<LabelStmt>(Last)) - Last = LS->getSubStmt(); - else if (auto *AS = dyn_cast<AttributedStmt>(Last)) - Last = AS->getSubStmt(); - else - return true; - if (auto *E = dyn_cast_if_present<Expr>(Last)) - return TraverseStmt(E, Queue); + if (auto *Last = + dyn_cast_if_present<ValueStmt>(SE->getSubStmt()->body_back())) + return TraverseStmt(Last->getExprStmt(), Queue); return true; } 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 e62b16540f4901..9eadd2d731da58 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 @@ -99,11 +99,29 @@ void f8(int arg) { return; } -int get_value(); +void f9(int x, int y) { + if (({ x = 1; ({ y = 2; }); })) + // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: an assignment within an 'if' condition is bug-prone [bugprone-assignment-in-if-condition] + return; +} + +void f10(int x) { + if (({ ({ x = 1; }); })) + // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: an assignment within an 'if' condition is bug-prone [bugprone-assignment-in-if-condition] + return; +} + +void f11(int arg) { + BrokenOperator bo1, bo2; + if (({ bo1 = arg; ({ bo2 = arg; }); })) + // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: an assignment within an 'if' condition is bug-prone [bugprone-assignment-in-if-condition] + return; +} -void f9(int arg) { - if (({ [[clang::nomerge]] arg = get_value(); })) - // CHECK-MESSAGES: :[[@LINE-1]]:33: warning: an assignment within an 'if' condition is bug-prone [bugprone-assignment-in-if-condition] +void f12(int arg) { + BrokenOperator bo; + if (({ ({ bo = arg; }); })) + // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: an assignment within an 'if' condition is bug-prone [bugprone-assignment-in-if-condition] return; } @@ -146,6 +164,17 @@ void awesome_f6(int arg) { return; } +void awesome_f7(int x, int y) { + if (({ x = 1; ({ y = 2; y; }); })) + return; +} + +void awesome_f8(int arg) { + BrokenOperator bo; + if (({ bo = arg; ({ bo = 2; bo.d; }); })) + return; +} + template <typename Func> bool exec(Func F) { return F(); } void lambda_if() { _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
