[clang] [clang-tools-extra] [analyzer] Remove alpha.core.IdenticalExpr Checker (PR #114715)
https://github.com/vabridgers closed https://github.com/llvm/llvm-project/pull/114715 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [analyzer] Remove alpha.core.IdenticalExpr Checker (PR #114715)
vabridgers wrote: Hi @HerrCai0907 and @5chmidti , I see that @HerrCai0907 approved the review. Is it ok if I merge the change? Just wanted to make sure I'm not missing any comments specifically from @5chmidti . Thanks! https://github.com/llvm/llvm-project/pull/114715 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [analyzer] Remove alpha.core.IdenticalExpr Checker (PR #114715)
@@ -102,6 +102,210 @@ void BranchCloneCheck::registerMatchers(MatchFinder
*Finder) {
this);
Finder->addMatcher(switchStmt().bind("switch"), this);
Finder->addMatcher(conditionalOperator().bind("condOp"), this);
+
Finder->addMatcher(ifStmt(hasDescendant(ifStmt())).bind("ifWithDescendantIf"),
vabridgers wrote:
Thanks @5chmidti. I think I had resolved the comment from @HerrCai0907 by
narrowing the match. Would you agree that's sufficient, or would you like to
see an improvement in this area before moving forward here? Thanks
https://github.com/llvm/llvm-project/pull/114715
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [analyzer] Remove alpha.core.IdenticalExpr Checker (PR #114715)
https://github.com/vabridgers updated
https://github.com/llvm/llvm-project/pull/114715
>From 838893009167d36e8293e4a4c2be417b56ef01df Mon Sep 17 00:00:00 2001
From: Vince Bridgers
Date: Thu, 7 Nov 2024 01:58:21 +0100
Subject: [PATCH] [analyzer] Port alpha.core.IdenticalExpr to Tidy checks and
remove
This change removes the alpha.core.IdenticalExpr static analysis
checker since it's checks are present in the clang-tidy checks
misc-redundant-expression and bugprone-branch-clone. This check was
implemented as a static analysis check using AST matching, and since
alpha and duplicated in 2 clang-tidy checks may be removed. The
existing LIT test was checked case by case, and the tidy checks
were improved to maintain alpha.core.IdenticalExpr features.
---
.../clang-tidy/bugprone/BranchCloneCheck.cpp | 221
.../misc/RedundantExpressionCheck.cpp | 70 ++-
clang-tools-extra/docs/ReleaseNotes.rst | 9 +
.../checks/bugprone/branch-clone.rst | 21 +-
.../checks/misc/redundant-expression.rst | 19 +
.../bugprone/alpha-core-identicalexpr.cpp | 335 +++
clang/docs/ReleaseNotes.rst | 4 +
clang/docs/analyzer/checkers.rst | 30 -
.../clang/StaticAnalyzer/Checkers/Checkers.td | 4 -
.../StaticAnalyzer/Checkers/CMakeLists.txt| 1 -
.../Checkers/IdenticalExprChecker.cpp | 519 --
11 files changed, 570 insertions(+), 663 deletions(-)
rename clang/test/Analysis/identical-expressions.cpp =>
clang-tools-extra/test/clang-tidy/checkers/bugprone/alpha-core-identicalexpr.cpp
(57%)
delete mode 100644 clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
diff --git a/clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
b/clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
index 356acf968db921..a7e25141b3fe26 100644
--- a/clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
@@ -102,6 +102,212 @@ void BranchCloneCheck::registerMatchers(MatchFinder
*Finder) {
this);
Finder->addMatcher(switchStmt().bind("switch"), this);
Finder->addMatcher(conditionalOperator().bind("condOp"), this);
+ Finder->addMatcher(
+ ifStmt((hasThen(hasDescendant(ifStmt().bind("ifWithDescendantIf"),
+ this);
+}
+
+/// Determines whether two statement trees are identical regarding
+/// operators and symbols.
+///
+/// Exceptions: expressions containing macros or functions with possible side
+/// effects are never considered identical.
+/// Limitations: (t + u) and (u + t) are not considered identical.
+/// t*(u + t) and t*u + t*t are not considered identical.
+///
+static bool isIdenticalStmt(const ASTContext &Ctx, const Stmt *Stmt1,
+const Stmt *Stmt2, bool IgnoreSideEffects) {
+
+ if (!Stmt1 || !Stmt2)
+return !Stmt1 && !Stmt2;
+
+ // If Stmt1 & Stmt2 are of different class then they are not
+ // identical statements.
+ if (Stmt1->getStmtClass() != Stmt2->getStmtClass())
+return false;
+
+ const auto *Expr1 = dyn_cast(Stmt1);
+ const auto *Expr2 = dyn_cast(Stmt2);
+
+ if (Expr1 && Expr2) {
+// If Stmt1 has side effects then don't warn even if expressions
+// are identical.
+if (!IgnoreSideEffects && Expr1->HasSideEffects(Ctx) &&
+Expr2->HasSideEffects(Ctx))
+ return false;
+// If either expression comes from a macro then don't warn even if
+// the expressions are identical.
+if ((Expr1->getExprLoc().isMacroID()) || (Expr2->getExprLoc().isMacroID()))
+ return false;
+
+// If all children of two expressions are identical, return true.
+Expr::const_child_iterator I1 = Expr1->child_begin();
+Expr::const_child_iterator I2 = Expr2->child_begin();
+while (I1 != Expr1->child_end() && I2 != Expr2->child_end()) {
+ if (!isIdenticalStmt(Ctx, *I1, *I2, IgnoreSideEffects))
+return false;
+ ++I1;
+ ++I2;
+}
+// If there are different number of children in the statements, return
+// false.
+if (I1 != Expr1->child_end())
+ return false;
+if (I2 != Expr2->child_end())
+ return false;
+ }
+
+ switch (Stmt1->getStmtClass()) {
+ default:
+return false;
+ case Stmt::CallExprClass:
+ case Stmt::ArraySubscriptExprClass:
+ case Stmt::ArraySectionExprClass:
+ case Stmt::OMPArrayShapingExprClass:
+ case Stmt::OMPIteratorExprClass:
+ case Stmt::ImplicitCastExprClass:
+ case Stmt::ParenExprClass:
+ case Stmt::BreakStmtClass:
+ case Stmt::ContinueStmtClass:
+ case Stmt::NullStmtClass:
+return true;
+ case Stmt::CStyleCastExprClass: {
+const auto *CastExpr1 = cast(Stmt1);
+const auto *CastExpr2 = cast(Stmt2);
+
+return CastExpr1->getTypeAsWritten() == CastExpr2->getTypeAsWritten();
+ }
+ case Stmt::ReturnStmtClass: {
+const auto *ReturnStmt1 = cast(Stmt1);
+const auto *ReturnStmt2 = cast(Stmt2);
+
+return isIdenticalStmt(Ctx, ReturnStmt1->getRetValue
[clang] [clang-tools-extra] [analyzer] Remove alpha.core.IdenticalExpr Checker (PR #114715)
@@ -102,6 +102,211 @@ void BranchCloneCheck::registerMatchers(MatchFinder
*Finder) {
this);
Finder->addMatcher(switchStmt().bind("switch"), this);
Finder->addMatcher(conditionalOperator().bind("condOp"), this);
+ Finder->addMatcher(
+ ifStmt((hasThen(hasDescendant(ifStmt().bind("ifWithDescendantIf"),
+ this);
+}
+
+/// Determines whether two statement trees are identical regarding
+/// operators and symbols.
+///
+/// Exceptions: expressions containing macros or functions with possible side
+/// effects are never considered identical.
+/// Limitations: (t + u) and (u + t) are not considered identical.
+/// t*(u + t) and t*u + t*t are not considered identical.
+///
+static bool isIdenticalStmt(const ASTContext &Ctx, const Stmt *Stmt1,
+const Stmt *Stmt2, bool IgnoreSideEffects) {
+
+ if (!Stmt1 || !Stmt2)
+return !Stmt1 && !Stmt2;
+
+ // If Stmt1 & Stmt2 are of different class then they are not
+ // identical statements.
+ if (Stmt1->getStmtClass() != Stmt2->getStmtClass())
+return false;
+
+ const auto *Expr1 = dyn_cast(Stmt1);
+ const auto *Expr2 = dyn_cast(Stmt2);
+
+ if (Expr1 && Expr2) {
+// If Stmt1 has side effects then don't warn even if expressions
+// are identical.
+if (!IgnoreSideEffects && Expr1->HasSideEffects(Ctx))
+ return false;
+// If either expression comes from a macro then don't warn even if
+// the expressions are identical.
+if ((Expr1->getExprLoc().isMacroID()) || (Expr2->getExprLoc().isMacroID()))
+ return false;
+
+// If all children of two expressions are identical, return true.
+Expr::const_child_iterator I1 = Expr1->child_begin();
+Expr::const_child_iterator I2 = Expr2->child_begin();
+while (I1 != Expr1->child_end() && I2 != Expr2->child_end()) {
+ if (!*I1 || !*I2 || !isIdenticalStmt(Ctx, *I1, *I2, IgnoreSideEffects))
HerrCai0907 wrote:
`!*I1 || !*I2` already be handled in `isIdenticalStmt`
https://github.com/llvm/llvm-project/pull/114715
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [analyzer] Remove alpha.core.IdenticalExpr Checker (PR #114715)
@@ -102,6 +102,211 @@ void BranchCloneCheck::registerMatchers(MatchFinder
*Finder) {
this);
Finder->addMatcher(switchStmt().bind("switch"), this);
Finder->addMatcher(conditionalOperator().bind("condOp"), this);
+ Finder->addMatcher(
+ ifStmt((hasThen(hasDescendant(ifStmt().bind("ifWithDescendantIf"),
+ this);
+}
+
+/// Determines whether two statement trees are identical regarding
+/// operators and symbols.
+///
+/// Exceptions: expressions containing macros or functions with possible side
+/// effects are never considered identical.
+/// Limitations: (t + u) and (u + t) are not considered identical.
+/// t*(u + t) and t*u + t*t are not considered identical.
+///
+static bool isIdenticalStmt(const ASTContext &Ctx, const Stmt *Stmt1,
+const Stmt *Stmt2, bool IgnoreSideEffects) {
+
+ if (!Stmt1 || !Stmt2)
+return !Stmt1 && !Stmt2;
+
+ // If Stmt1 & Stmt2 are of different class then they are not
+ // identical statements.
+ if (Stmt1->getStmtClass() != Stmt2->getStmtClass())
+return false;
+
+ const auto *Expr1 = dyn_cast(Stmt1);
+ const auto *Expr2 = dyn_cast(Stmt2);
+
+ if (Expr1 && Expr2) {
+// If Stmt1 has side effects then don't warn even if expressions
+// are identical.
+if (!IgnoreSideEffects && Expr1->HasSideEffects(Ctx))
HerrCai0907 wrote:
How about Stmt2 has side effects?
https://github.com/llvm/llvm-project/pull/114715
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [analyzer] Remove alpha.core.IdenticalExpr Checker (PR #114715)
https://github.com/HerrCai0907 approved this pull request. https://github.com/llvm/llvm-project/pull/114715 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [analyzer] Remove alpha.core.IdenticalExpr Checker (PR #114715)
https://github.com/5chmidti edited https://github.com/llvm/llvm-project/pull/114715 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [analyzer] Remove alpha.core.IdenticalExpr Checker (PR #114715)
https://github.com/5chmidti edited https://github.com/llvm/llvm-project/pull/114715 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [analyzer] Remove alpha.core.IdenticalExpr Checker (PR #114715)
@@ -102,6 +102,210 @@ void BranchCloneCheck::registerMatchers(MatchFinder
*Finder) {
this);
Finder->addMatcher(switchStmt().bind("switch"), this);
Finder->addMatcher(conditionalOperator().bind("condOp"), this);
+
Finder->addMatcher(ifStmt(hasDescendant(ifStmt())).bind("ifWithDescendantIf"),
5chmidti wrote:
The current matcher will check for `if`s as descendants, not only as direct
children. So code like
```c++
void foo() {
if (true) {
[]() {
if (false) {
}
};
}
}
```
will still produce a match, that you filter out later, instead of in the
matcher.
A matcher like `ifStmt(anyOf(has(ifStmt().bind("inner_if")),
has(compoundStmt(has(ifStmt().bind("inner_if").bind("ifWithDescendantIf")`
will ensure that only direct children are looked at. To enforce the requirement
of the first statement inside a compound statement, adding a custom matcher
would be best IMO.
https://github.com/llvm/llvm-project/pull/114715
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [analyzer] Remove alpha.core.IdenticalExpr Checker (PR #114715)
vabridgers wrote: Ping! Any more comments on this change? Thanks https://github.com/llvm/llvm-project/pull/114715 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [analyzer] Remove alpha.core.IdenticalExpr Checker (PR #114715)
vabridgers wrote: Thanks @HerrCai0907 and @5chmidti for the constructive comments. I believe all have been addressed and a way forward has been described. Please let me know how we can move this forward. Thank you! https://github.com/llvm/llvm-project/pull/114715 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [analyzer] Remove alpha.core.IdenticalExpr Checker (PR #114715)
https://github.com/vabridgers updated
https://github.com/llvm/llvm-project/pull/114715
>From 6bcbe9f09060dc2bdcdb9a9c6e576466e24e08cb Mon Sep 17 00:00:00 2001
From: Vince Bridgers
Date: Thu, 7 Nov 2024 01:58:21 +0100
Subject: [PATCH] [analyzer] Port alpha.core.IdenticalExpr to Tidy checks and
remove
This change removes the alpha.core.IdenticalExpr static analysis
checker since it's checks are present in the clang-tidy checks
misc-redundant-expression and bugprone-branch-clone. This check was
implemented as a static analysis check using AST matching, and since
alpha and duplicated in 2 clang-tidy checks may be removed. The
existing LIT test was checked case by case, and the tidy checks
were improved to maintain alpha.core.IdenticalExpr features.
---
.../clang-tidy/bugprone/BranchCloneCheck.cpp | 220
.../misc/RedundantExpressionCheck.cpp | 70 ++-
clang-tools-extra/docs/ReleaseNotes.rst | 9 +
.../checks/bugprone/branch-clone.rst | 21 +-
.../checks/misc/redundant-expression.rst | 19 +
.../bugprone/alpha-core-identicalexpr.cpp | 335 +++
clang/docs/ReleaseNotes.rst | 4 +
clang/docs/analyzer/checkers.rst | 30 -
.../clang/StaticAnalyzer/Checkers/Checkers.td | 4 -
.../StaticAnalyzer/Checkers/CMakeLists.txt| 1 -
.../Checkers/IdenticalExprChecker.cpp | 519 --
11 files changed, 569 insertions(+), 663 deletions(-)
rename clang/test/Analysis/identical-expressions.cpp =>
clang-tools-extra/test/clang-tidy/checkers/bugprone/alpha-core-identicalexpr.cpp
(57%)
delete mode 100644 clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
diff --git a/clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
b/clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
index 356acf968db921..9702f0632ead59 100644
--- a/clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
@@ -102,6 +102,211 @@ void BranchCloneCheck::registerMatchers(MatchFinder
*Finder) {
this);
Finder->addMatcher(switchStmt().bind("switch"), this);
Finder->addMatcher(conditionalOperator().bind("condOp"), this);
+ Finder->addMatcher(
+ ifStmt((hasThen(hasDescendant(ifStmt().bind("ifWithDescendantIf"),
+ this);
+}
+
+/// Determines whether two statement trees are identical regarding
+/// operators and symbols.
+///
+/// Exceptions: expressions containing macros or functions with possible side
+/// effects are never considered identical.
+/// Limitations: (t + u) and (u + t) are not considered identical.
+/// t*(u + t) and t*u + t*t are not considered identical.
+///
+static bool isIdenticalStmt(const ASTContext &Ctx, const Stmt *Stmt1,
+const Stmt *Stmt2, bool IgnoreSideEffects) {
+
+ if (!Stmt1 || !Stmt2)
+return !Stmt1 && !Stmt2;
+
+ // If Stmt1 & Stmt2 are of different class then they are not
+ // identical statements.
+ if (Stmt1->getStmtClass() != Stmt2->getStmtClass())
+return false;
+
+ const auto *Expr1 = dyn_cast(Stmt1);
+ const auto *Expr2 = dyn_cast(Stmt2);
+
+ if (Expr1 && Expr2) {
+// If Stmt1 has side effects then don't warn even if expressions
+// are identical.
+if (!IgnoreSideEffects && Expr1->HasSideEffects(Ctx))
+ return false;
+// If either expression comes from a macro then don't warn even if
+// the expressions are identical.
+if ((Expr1->getExprLoc().isMacroID()) || (Expr2->getExprLoc().isMacroID()))
+ return false;
+
+// If all children of two expressions are identical, return true.
+Expr::const_child_iterator I1 = Expr1->child_begin();
+Expr::const_child_iterator I2 = Expr2->child_begin();
+while (I1 != Expr1->child_end() && I2 != Expr2->child_end()) {
+ if (!*I1 || !*I2 || !isIdenticalStmt(Ctx, *I1, *I2, IgnoreSideEffects))
+return false;
+ ++I1;
+ ++I2;
+}
+// If there are different number of children in the statements, return
+// false.
+if (I1 != Expr1->child_end())
+ return false;
+if (I2 != Expr2->child_end())
+ return false;
+ }
+
+ switch (Stmt1->getStmtClass()) {
+ default:
+return false;
+ case Stmt::CallExprClass:
+ case Stmt::ArraySubscriptExprClass:
+ case Stmt::ArraySectionExprClass:
+ case Stmt::OMPArrayShapingExprClass:
+ case Stmt::OMPIteratorExprClass:
+ case Stmt::ImplicitCastExprClass:
+ case Stmt::ParenExprClass:
+ case Stmt::BreakStmtClass:
+ case Stmt::ContinueStmtClass:
+ case Stmt::NullStmtClass:
+return true;
+ case Stmt::CStyleCastExprClass: {
+const auto *CastExpr1 = cast(Stmt1);
+const auto *CastExpr2 = cast(Stmt2);
+
+return CastExpr1->getTypeAsWritten() == CastExpr2->getTypeAsWritten();
+ }
+ case Stmt::ReturnStmtClass: {
+const auto *ReturnStmt1 = cast(Stmt1);
+const auto *ReturnStmt2 = cast(Stmt2);
+
+return isIdenticalStmt(Ctx, ReturnStmt1->getRetValue(),
+
[clang] [clang-tools-extra] [analyzer] Remove alpha.core.IdenticalExpr Checker (PR #114715)
@@ -269,6 +473,21 @@ void BranchCloneCheck::check(const
MatchFinder::MatchResult &Result) {
return;
}
+ if (const auto *IS = Result.Nodes.getNodeAs("ifWithDescendantIf")) {
+const Stmt *Then = IS->getThen();
+auto CS = dyn_cast(Then);
+if (CS && (!CS->body_empty())) {
+ const auto *InnerIf = dyn_cast(*CS->body_begin());
+ if (InnerIf && isIdenticalStmt(Context, IS->getCond(),
InnerIf->getCond(),
+ /*IgnoreSideEffects=*/false)) {
HerrCai0907 wrote:
I mean we can do this check in ast_matcher to reduce the cost
https://github.com/llvm/llvm-project/pull/114715
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [analyzer] Remove alpha.core.IdenticalExpr Checker (PR #114715)
@@ -102,6 +102,210 @@ void BranchCloneCheck::registerMatchers(MatchFinder
*Finder) {
this);
Finder->addMatcher(switchStmt().bind("switch"), this);
Finder->addMatcher(conditionalOperator().bind("condOp"), this);
+
Finder->addMatcher(ifStmt(hasDescendant(ifStmt())).bind("ifWithDescendantIf"),
vabridgers wrote:
Hello @HerrCai0907 , I do not fully understand your suggestion. Could you
describe what you mean by "move the later check to here"? Thanks for taking the
time to review. Best!
https://github.com/llvm/llvm-project/pull/114715
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [analyzer] Remove alpha.core.IdenticalExpr Checker (PR #114715)
@@ -102,6 +102,210 @@ void BranchCloneCheck::registerMatchers(MatchFinder
*Finder) {
this);
Finder->addMatcher(switchStmt().bind("switch"), this);
Finder->addMatcher(conditionalOperator().bind("condOp"), this);
+
Finder->addMatcher(ifStmt(hasDescendant(ifStmt())).bind("ifWithDescendantIf"),
HerrCai0907 wrote:
`hasDescendant` will match lots of unrelated cases which will slow the speed.
move the later check to here
https://github.com/llvm/llvm-project/pull/114715
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [analyzer] Remove alpha.core.IdenticalExpr Checker (PR #114715)
https://github.com/vabridgers updated
https://github.com/llvm/llvm-project/pull/114715
>From 48389a341ece8546261b84de7af79cbb80829254 Mon Sep 17 00:00:00 2001
From: Vince Bridgers
Date: Thu, 7 Nov 2024 01:58:21 +0100
Subject: [PATCH] [analyzer] Port alpha.core.IdenticalExpr to Tidy checks and
remove
This change removes the alpha.core.IdenticalExpr static analysis
checker since it's checks are present in the clang-tidy checks
misc-redundant-expression and bugprone-branch-clone. This check was
implemented as a static analysis check using AST matching, and since
alpha and duplicated in 2 clang-tidy checks may be removed. The
existing LIT test was checked case by case, and the tidy checks
were improved to maintain alpha.core.IdenticalExpr features.
---
.../clang-tidy/bugprone/BranchCloneCheck.cpp | 217
.../misc/RedundantExpressionCheck.cpp | 70 ++-
clang-tools-extra/docs/ReleaseNotes.rst | 9 +
.../checks/bugprone/branch-clone.rst | 21 +-
.../checks/misc/redundant-expression.rst | 19 +
.../bugprone/alpha-core-identicalexpr.cpp | 335 +++
clang/docs/ReleaseNotes.rst | 4 +
clang/docs/analyzer/checkers.rst | 30 -
.../clang/StaticAnalyzer/Checkers/Checkers.td | 4 -
.../StaticAnalyzer/Checkers/CMakeLists.txt| 1 -
.../Checkers/IdenticalExprChecker.cpp | 519 --
11 files changed, 566 insertions(+), 663 deletions(-)
rename clang/test/Analysis/identical-expressions.cpp =>
clang-tools-extra/test/clang-tidy/checkers/bugprone/alpha-core-identicalexpr.cpp
(57%)
delete mode 100644 clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
diff --git a/clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
b/clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
index 356acf968db921..2a94279d7b48da 100644
--- a/clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
@@ -102,6 +102,208 @@ void BranchCloneCheck::registerMatchers(MatchFinder
*Finder) {
this);
Finder->addMatcher(switchStmt().bind("switch"), this);
Finder->addMatcher(conditionalOperator().bind("condOp"), this);
+
Finder->addMatcher(ifStmt(hasDescendant(ifStmt())).bind("ifWithDescendantIf"),
+ this);
+}
+
+/// Determines whether two statement trees are identical regarding
+/// operators and symbols.
+///
+/// Exceptions: expressions containing macros or functions with possible side
+/// effects are never considered identical.
+/// Limitations: (t + u) and (u + t) are not considered identical.
+/// t*(u + t) and t*u + t*t are not considered identical.
+///
+static bool isIdenticalStmt(const ASTContext &Ctx, const Stmt *Stmt1,
+const Stmt *Stmt2, bool IgnoreSideEffects) {
+
+ if (!Stmt1 || !Stmt2)
+return !Stmt1 && !Stmt2;
+
+ // If Stmt1 & Stmt2 are of different class then they are not
+ // identical statements.
+ if (Stmt1->getStmtClass() != Stmt2->getStmtClass())
+return false;
+
+ const auto *Expr1 = dyn_cast(Stmt1);
+ const auto *Expr2 = dyn_cast(Stmt2);
+
+ if (Expr1 && Expr2) {
+// If Stmt1 has side effects then don't warn even if expressions
+// are identical.
+if (!IgnoreSideEffects && Expr1->HasSideEffects(Ctx))
+ return false;
+// If either expression comes from a macro then don't warn even if
+// the expressions are identical.
+if ((Expr1->getExprLoc().isMacroID()) || (Expr2->getExprLoc().isMacroID()))
+ return false;
+
+// If all children of two expressions are identical, return true.
+Expr::const_child_iterator I1 = Expr1->child_begin();
+Expr::const_child_iterator I2 = Expr2->child_begin();
+while (I1 != Expr1->child_end() && I2 != Expr2->child_end()) {
+ if (!*I1 || !*I2 || !isIdenticalStmt(Ctx, *I1, *I2, IgnoreSideEffects))
+return false;
+ ++I1;
+ ++I2;
+}
+// If there are different number of children in the statements, return
+// false.
+if (I1 != Expr1->child_end())
+ return false;
+if (I2 != Expr2->child_end())
+ return false;
+ }
+
+ switch (Stmt1->getStmtClass()) {
+ default:
+return false;
+ case Stmt::CallExprClass:
+ case Stmt::ArraySubscriptExprClass:
+ case Stmt::ArraySectionExprClass:
+ case Stmt::OMPArrayShapingExprClass:
+ case Stmt::OMPIteratorExprClass:
+ case Stmt::ImplicitCastExprClass:
+ case Stmt::ParenExprClass:
+ case Stmt::BreakStmtClass:
+ case Stmt::ContinueStmtClass:
+ case Stmt::NullStmtClass:
+return true;
+ case Stmt::CStyleCastExprClass: {
+const auto *CastExpr1 = cast(Stmt1);
+const auto *CastExpr2 = cast(Stmt2);
+
+return CastExpr1->getTypeAsWritten() == CastExpr2->getTypeAsWritten();
+ }
+ case Stmt::ReturnStmtClass: {
+const auto *ReturnStmt1 = cast(Stmt1);
+const auto *ReturnStmt2 = cast(Stmt2);
+
+return isIdenticalStmt(Ctx, ReturnStmt1->getRetValue(),
+
[clang] [clang-tools-extra] [analyzer] Remove alpha.core.IdenticalExpr Checker (PR #114715)
vabridgers wrote: I'll follow up after this PR is merged with a subsequent PR to split this LIT. If that's not acceptable please reopen this comment. Thanks. https://github.com/llvm/llvm-project/pull/114715 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [analyzer] Remove alpha.core.IdenticalExpr Checker (PR #114715)
https://github.com/vabridgers updated
https://github.com/llvm/llvm-project/pull/114715
>From 6bcb40f63c1393f45a0acdbe3631b85e20d4e93c Mon Sep 17 00:00:00 2001
From: Vince Bridgers
Date: Thu, 7 Nov 2024 01:58:21 +0100
Subject: [PATCH] [analyzer] Port alpha.core.IdenticalExpr to Tidy checks and
remove
This change removes the alpha.core.IdenticalExpr static analysis
checker since it's checks are present in the clang-tidy checks
misc-redundant-expression and bugprone-branch-clone. This check was
implemented as a static analysis check using AST matching, and since
alpha and duplicated in 2 clang-tidy checks may be removed. The
existing LIT test was checked case by case, and the tidy checks
were improved to maintain alpha.core.IdenticalExpr features.
---
.../clang-tidy/bugprone/BranchCloneCheck.cpp | 221
.../misc/RedundantExpressionCheck.cpp | 70 ++-
clang-tools-extra/docs/ReleaseNotes.rst | 9 +
.../checks/bugprone/branch-clone.rst | 21 +-
.../checks/misc/redundant-expression.rst | 19 +
.../bugprone/alpha-core-identicalexpr.cpp | 335 +++
clang/docs/ReleaseNotes.rst | 4 +
clang/docs/analyzer/checkers.rst | 30 -
.../clang/StaticAnalyzer/Checkers/Checkers.td | 4 -
.../StaticAnalyzer/Checkers/CMakeLists.txt| 1 -
.../Checkers/IdenticalExprChecker.cpp | 519 --
11 files changed, 570 insertions(+), 663 deletions(-)
rename clang/test/Analysis/identical-expressions.cpp =>
clang-tools-extra/test/clang-tidy/checkers/bugprone/alpha-core-identicalexpr.cpp
(57%)
delete mode 100644 clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
diff --git a/clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
b/clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
index 356acf968db921..ff63926ccbcd6b 100644
--- a/clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
@@ -15,6 +15,10 @@
#include "clang/Lex/Lexer.h"
#include "llvm/Support/Casting.h"
+// #include
+// #include
+// #include
+
using namespace clang;
using namespace clang::ast_matchers;
@@ -102,6 +106,208 @@ void BranchCloneCheck::registerMatchers(MatchFinder
*Finder) {
this);
Finder->addMatcher(switchStmt().bind("switch"), this);
Finder->addMatcher(conditionalOperator().bind("condOp"), this);
+
Finder->addMatcher(ifStmt(hasDescendant(ifStmt())).bind("ifWithDescendantIf"),
+ this);
+}
+
+/// Determines whether two statement trees are identical regarding
+/// operators and symbols.
+///
+/// Exceptions: expressions containing macros or functions with possible side
+/// effects are never considered identical.
+/// Limitations: (t + u) and (u + t) are not considered identical.
+/// t*(u + t) and t*u + t*t are not considered identical.
+///
+static bool isIdenticalStmt(const ASTContext &Ctx, const Stmt *Stmt1,
+const Stmt *Stmt2, bool IgnoreSideEffects) {
+
+ if (!Stmt1 || !Stmt2)
+return !Stmt1 && !Stmt2;
+
+ // If Stmt1 & Stmt2 are of different class then they are not
+ // identical statements.
+ if (Stmt1->getStmtClass() != Stmt2->getStmtClass())
+return false;
+
+ const auto *Expr1 = dyn_cast(Stmt1);
+ const auto *Expr2 = dyn_cast(Stmt2);
+
+ if (Expr1 && Expr2) {
+// If Stmt1 has side effects then don't warn even if expressions
+// are identical.
+if (!IgnoreSideEffects && Expr1->HasSideEffects(Ctx))
+ return false;
+// If either expression comes from a macro then don't warn even if
+// the expressions are identical.
+if ((Expr1->getExprLoc().isMacroID()) || (Expr2->getExprLoc().isMacroID()))
+ return false;
+
+// If all children of two expressions are identical, return true.
+Expr::const_child_iterator I1 = Expr1->child_begin();
+Expr::const_child_iterator I2 = Expr2->child_begin();
+while (I1 != Expr1->child_end() && I2 != Expr2->child_end()) {
+ if (!*I1 || !*I2 || !isIdenticalStmt(Ctx, *I1, *I2, IgnoreSideEffects))
+return false;
+ ++I1;
+ ++I2;
+}
+// If there are different number of children in the statements, return
+// false.
+if (I1 != Expr1->child_end())
+ return false;
+if (I2 != Expr2->child_end())
+ return false;
+ }
+
+ switch (Stmt1->getStmtClass()) {
+ default:
+return false;
+ case Stmt::CallExprClass:
+ case Stmt::ArraySubscriptExprClass:
+ case Stmt::ArraySectionExprClass:
+ case Stmt::OMPArrayShapingExprClass:
+ case Stmt::OMPIteratorExprClass:
+ case Stmt::ImplicitCastExprClass:
+ case Stmt::ParenExprClass:
+ case Stmt::BreakStmtClass:
+ case Stmt::ContinueStmtClass:
+ case Stmt::NullStmtClass:
+return true;
+ case Stmt::CStyleCastExprClass: {
+const auto *CastExpr1 = cast(Stmt1);
+const auto *CastExpr2 = cast(Stmt2);
+
+return CastExpr1->getTypeAsWritten() == CastExpr2->getTypeAsWritten();
+ }
+ case Stmt
[clang] [clang-tools-extra] [analyzer] Remove alpha.core.IdenticalExpr Checker (PR #114715)
https://github.com/vabridgers updated
https://github.com/llvm/llvm-project/pull/114715
>From 699d4b89b23c5b3c1f55223d6983c19a26cf2741 Mon Sep 17 00:00:00 2001
From: Vince Bridgers
Date: Thu, 7 Nov 2024 01:58:21 +0100
Subject: [PATCH] [analyzer] Port alpha.core.IdenticalExpr to Tidy checks and
remove
This change removes the alpha.core.IdenticalExpr static analysis
checker since it's checks are present in the clang-tidy checks
misc-redundant-expression and bugprone-branch-clone. This check was
implemented as a static analysis check using AST matching, and since
alpha and duplicated in 2 clang-tidy checks may be removed. The
existing LIT test was checked case by case, and the tidy checks
were improved to maintain alpha.core.IdenticalExpr features.
---
.../clang-tidy/bugprone/BranchCloneCheck.cpp | 219
.../misc/RedundantExpressionCheck.cpp | 70 ++-
clang-tools-extra/docs/ReleaseNotes.rst | 9 +
.../checks/bugprone/branch-clone.rst | 21 +-
.../checks/misc/redundant-expression.rst | 19 +
.../bugprone/alpha-core-identicalexpr.cpp | 335 +++
clang/docs/ReleaseNotes.rst | 4 +
clang/docs/analyzer/checkers.rst | 30 -
.../clang/StaticAnalyzer/Checkers/Checkers.td | 4 -
.../StaticAnalyzer/Checkers/CMakeLists.txt| 1 -
.../Checkers/IdenticalExprChecker.cpp | 519 --
11 files changed, 568 insertions(+), 663 deletions(-)
rename clang/test/Analysis/identical-expressions.cpp =>
clang-tools-extra/test/clang-tidy/checkers/bugprone/alpha-core-identicalexpr.cpp
(57%)
delete mode 100644 clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
diff --git a/clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
b/clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
index 356acf968db921..4bb7e0fd2f92ca 100644
--- a/clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
@@ -102,6 +102,210 @@ void BranchCloneCheck::registerMatchers(MatchFinder
*Finder) {
this);
Finder->addMatcher(switchStmt().bind("switch"), this);
Finder->addMatcher(conditionalOperator().bind("condOp"), this);
+
Finder->addMatcher(ifStmt(hasDescendant(ifStmt())).bind("ifWithDescendantIf"),
+ this);
+}
+
+/// Determines whether two statement trees are identical regarding
+/// operators and symbols.
+///
+/// Exceptions: expressions containing macros or functions with possible side
+/// effects are never considered identical.
+/// Limitations: (t + u) and (u + t) are not considered identical.
+/// t*(u + t) and t*u + t*t are not considered identical.
+///
+static bool isIdenticalStmt(const ASTContext &Ctx, const Stmt *Stmt1,
+const Stmt *Stmt2, bool IgnoreSideEffects) {
+
+ if (!Stmt1 || !Stmt2)
+return !Stmt1 && !Stmt2;
+
+ // If Stmt1 & Stmt2 are of different class then they are not
+ // identical statements.
+ if (Stmt1->getStmtClass() != Stmt2->getStmtClass())
+return false;
+
+ const auto *Expr1 = dyn_cast(Stmt1);
+ const auto *Expr2 = dyn_cast(Stmt2);
+
+ if (Expr1 && Expr2) {
+// If Stmt1 has side effects then don't warn even if expressions
+// are identical.
+if (!IgnoreSideEffects && Expr1->HasSideEffects(Ctx))
+ return false;
+// If either expression comes from a macro then don't warn even if
+// the expressions are identical.
+if ((Expr1->getExprLoc().isMacroID()) || (Expr2->getExprLoc().isMacroID()))
+ return false;
+
+// If all children of two expressions are identical, return true.
+Expr::const_child_iterator I1 = Expr1->child_begin();
+Expr::const_child_iterator I2 = Expr2->child_begin();
+while (I1 != Expr1->child_end() && I2 != Expr2->child_end()) {
+ if (!*I1 || !*I2 || !isIdenticalStmt(Ctx, *I1, *I2, IgnoreSideEffects))
+return false;
+ ++I1;
+ ++I2;
+}
+// If there are different number of children in the statements, return
+// false.
+if (I1 != Expr1->child_end())
+ return false;
+if (I2 != Expr2->child_end())
+ return false;
+ }
+
+ switch (Stmt1->getStmtClass()) {
+ default:
+return false;
+ case Stmt::CallExprClass:
+ case Stmt::ArraySubscriptExprClass:
+ case Stmt::ArraySectionExprClass:
+ case Stmt::OMPArrayShapingExprClass:
+ case Stmt::OMPIteratorExprClass:
+ case Stmt::ImplicitCastExprClass:
+ case Stmt::ParenExprClass:
+ case Stmt::BreakStmtClass:
+ case Stmt::ContinueStmtClass:
+ case Stmt::NullStmtClass:
+return true;
+ case Stmt::CStyleCastExprClass: {
+const auto *CastExpr1 = cast(Stmt1);
+const auto *CastExpr2 = cast(Stmt2);
+
+return CastExpr1->getTypeAsWritten() == CastExpr2->getTypeAsWritten();
+ }
+ case Stmt::ReturnStmtClass: {
+const auto *ReturnStmt1 = cast(Stmt1);
+const auto *ReturnStmt2 = cast(Stmt2);
+
+return isIdenticalStmt(Ctx, ReturnStmt1->getRetValue(),
+
[clang] [clang-tools-extra] [analyzer] Remove alpha.core.IdenticalExpr Checker (PR #114715)
vabridgers wrote: Thanks @5chmidti for the constructive comments. I believe those have been addressed. I'll follow up after this PR is approved and merged to split the LIT appropriately. Thanks for understanding this approach, it was the only one I could think of where the reviewers could transparently observe all features were maintained from the older checker. I'll happily make any other suggested improvements if I've missed something. Best https://github.com/llvm/llvm-project/pull/114715 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [analyzer] Remove alpha.core.IdenticalExpr Checker (PR #114715)
@@ -866,19 +866,16 @@ void
RedundantExpressionCheck::registerMatchers(MatchFinder *Finder) {
// Binary with equivalent operands, like (X != 2 && X != 2).
Finder->addMatcher(
traverse(TK_AsIs,
- binaryOperator(
- anyOf(isComparisonOperator(),
- hasAnyOperatorName("-", "/", "%", "|", "&", "^", "&&",
-"||", "=")),
- operandsAreEquivalent(),
- // Filter noisy false positives.
- unless(isInTemplateInstantiation()),
- unless(binaryOperatorIsInMacro()),
- unless(hasType(realFloatingPointType())),
- unless(hasEitherOperand(hasType(realFloatingPointType(,
- unless(hasLHS(AnyLiteralExpr)),
5chmidti wrote:
`AnyLiteralExpr` is now unused, please remove it.
https://github.com/llvm/llvm-project/pull/114715
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [analyzer] Remove alpha.core.IdenticalExpr Checker (PR #114715)
@@ -269,6 +472,23 @@ void BranchCloneCheck::check(const
MatchFinder::MatchResult &Result) {
return;
}
+ if (const auto *IS = Result.Nodes.getNodeAs("ifWithDescendantIf")) {
+const Stmt *Then = IS->getThen();
+if (const CompoundStmt *CS = dyn_cast(Then)) {
+ if (!CS->body_empty()) {
5chmidti wrote:
These two `if` statements could be one
https://github.com/llvm/llvm-project/pull/114715
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [analyzer] Remove alpha.core.IdenticalExpr Checker (PR #114715)
@@ -32,9 +32,28 @@ If this is the intended behavior, then there is no reason to use a conditional statement; otherwise the issue can be solved by fixing the branch that is handled incorrectly. -The check also detects repeated branches in longer ``if/else if/else`` chains +The check detects repeated branches in longer ``if/else if/else`` chains where it would be even harder to notice the problem. +The check also detects repeated inner and outer if statements that may +be a result of a copy-paste error. This check cannot currently detect +identical inner and outer if statements if code is between the if 5chmidti wrote: ``` ``if`` statements ``` https://github.com/llvm/llvm-project/pull/114715 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [analyzer] Remove alpha.core.IdenticalExpr Checker (PR #114715)
@@ -102,6 +102,209 @@ void BranchCloneCheck::registerMatchers(MatchFinder
*Finder) {
this);
Finder->addMatcher(switchStmt().bind("switch"), this);
Finder->addMatcher(conditionalOperator().bind("condOp"), this);
+
Finder->addMatcher(ifStmt(hasDescendant(ifStmt())).bind("ifWithDescendantIf"),
+ this);
+}
+
+/// Determines whether two statement trees are identical regarding
+/// operators and symbols.
+///
+/// Exceptions: expressions containing macros or functions with possible side
+/// effects are never considered identical.
+/// Limitations: (t + u) and (u + t) are not considered identical.
+/// t*(u + t) and t*u + t*t are not considered identical.
+///
+static bool isIdenticalStmt(const ASTContext &Ctx, const Stmt *Stmt1,
+const Stmt *Stmt2, bool IgnoreSideEffects) {
+
+ if (!Stmt1 || !Stmt2) {
+return !Stmt1 && !Stmt2;
+ }
5chmidti wrote:
Nit: single statement `if` -> no brace
https://github.com/llvm/llvm-project/pull/114715
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [analyzer] Remove alpha.core.IdenticalExpr Checker (PR #114715)
@@ -102,6 +102,209 @@ void BranchCloneCheck::registerMatchers(MatchFinder
*Finder) {
this);
Finder->addMatcher(switchStmt().bind("switch"), this);
Finder->addMatcher(conditionalOperator().bind("condOp"), this);
+
Finder->addMatcher(ifStmt(hasDescendant(ifStmt())).bind("ifWithDescendantIf"),
+ this);
+}
+
+/// Determines whether two statement trees are identical regarding
+/// operators and symbols.
+///
+/// Exceptions: expressions containing macros or functions with possible side
+/// effects are never considered identical.
+/// Limitations: (t + u) and (u + t) are not considered identical.
+/// t*(u + t) and t*u + t*t are not considered identical.
+///
+static bool isIdenticalStmt(const ASTContext &Ctx, const Stmt *Stmt1,
+const Stmt *Stmt2, bool IgnoreSideEffects) {
+
+ if (!Stmt1 || !Stmt2) {
+return !Stmt1 && !Stmt2;
+ }
+
+ // If Stmt1 & Stmt2 are of different class then they are not
+ // identical statements.
+ if (Stmt1->getStmtClass() != Stmt2->getStmtClass())
+return false;
+
+ const auto *Expr1 = dyn_cast(Stmt1);
+ const auto *Expr2 = dyn_cast(Stmt2);
+
+ if (Expr1 && Expr2) {
+// If Stmt1 has side effects then don't warn even if expressions
+// are identical.
+if (!IgnoreSideEffects && Expr1->HasSideEffects(Ctx))
+ return false;
+// If either expression comes from a macro then don't warn even if
+// the expressions are identical.
+if ((Expr1->getExprLoc().isMacroID()) || (Expr2->getExprLoc().isMacroID()))
+ return false;
+
+// If all children of two expressions are identical, return true.
+Expr::const_child_iterator I1 = Expr1->child_begin();
+Expr::const_child_iterator I2 = Expr2->child_begin();
+while (I1 != Expr1->child_end() && I2 != Expr2->child_end()) {
+ if (!*I1 || !*I2 || !isIdenticalStmt(Ctx, *I1, *I2, IgnoreSideEffects))
+return false;
+ ++I1;
+ ++I2;
+}
+// If there are different number of children in the statements, return
+// false.
+if (I1 != Expr1->child_end())
+ return false;
+if (I2 != Expr2->child_end())
+ return false;
+ }
+
+ switch (Stmt1->getStmtClass()) {
+ default:
+return false;
+ case Stmt::CallExprClass:
+ case Stmt::ArraySubscriptExprClass:
+ case Stmt::ArraySectionExprClass:
+ case Stmt::OMPArrayShapingExprClass:
+ case Stmt::OMPIteratorExprClass:
+ case Stmt::ImplicitCastExprClass:
+ case Stmt::ParenExprClass:
+ case Stmt::BreakStmtClass:
+ case Stmt::ContinueStmtClass:
+ case Stmt::NullStmtClass:
+return true;
+ case Stmt::CStyleCastExprClass: {
+const auto *CastExpr1 = cast(Stmt1);
+const auto *CastExpr2 = cast(Stmt2);
+
+return CastExpr1->getTypeAsWritten() == CastExpr2->getTypeAsWritten();
+ }
+ case Stmt::ReturnStmtClass: {
+const auto *ReturnStmt1 = cast(Stmt1);
+const auto *ReturnStmt2 = cast(Stmt2);
+
+return isIdenticalStmt(Ctx, ReturnStmt1->getRetValue(),
+ ReturnStmt2->getRetValue(), IgnoreSideEffects);
+ }
+ case Stmt::ForStmtClass: {
+const auto *ForStmt1 = cast(Stmt1);
+const auto *ForStmt2 = cast(Stmt2);
+
+if (!isIdenticalStmt(Ctx, ForStmt1->getInit(), ForStmt2->getInit(),
+ IgnoreSideEffects))
+ return false;
+if (!isIdenticalStmt(Ctx, ForStmt1->getCond(), ForStmt2->getCond(),
+ IgnoreSideEffects))
+ return false;
+if (!isIdenticalStmt(Ctx, ForStmt1->getInc(), ForStmt2->getInc(),
+ IgnoreSideEffects))
+ return false;
+if (!isIdenticalStmt(Ctx, ForStmt1->getBody(), ForStmt2->getBody(),
+ IgnoreSideEffects))
+ return false;
+return true;
+ }
+ case Stmt::DoStmtClass: {
+const auto *DStmt1 = cast(Stmt1);
+const auto *DStmt2 = cast(Stmt2);
+
+if (!isIdenticalStmt(Ctx, DStmt1->getCond(), DStmt2->getCond(),
+ IgnoreSideEffects))
+ return false;
+if (!isIdenticalStmt(Ctx, DStmt1->getBody(), DStmt2->getBody(),
+ IgnoreSideEffects))
+ return false;
+return true;
+ }
+ case Stmt::WhileStmtClass: {
+const auto *WStmt1 = cast(Stmt1);
+const auto *WStmt2 = cast(Stmt2);
+
+if (!isIdenticalStmt(Ctx, WStmt1->getCond(), WStmt2->getCond(),
+ IgnoreSideEffects))
+ return false;
+if (!isIdenticalStmt(Ctx, WStmt1->getBody(), WStmt2->getBody(),
+ IgnoreSideEffects))
+ return false;
+return true;
+ }
+ case Stmt::IfStmtClass: {
+const auto *IStmt1 = cast(Stmt1);
+const auto *IStmt2 = cast(Stmt2);
+
+if (!isIdenticalStmt(Ctx, IStmt1->getCond(), IStmt2->getCond(),
+ IgnoreSideEffects))
+ return false;
+if (!isIdenticalStmt(Ctx, IStmt1->getThen(), IStmt2->getThen(),
+ IgnoreSideEffects))
+ return false;
+if (!isIdenticalStmt
[clang] [clang-tools-extra] [analyzer] Remove alpha.core.IdenticalExpr Checker (PR #114715)
@@ -1238,6 +1235,59 @@ void RedundantExpressionCheck::check(const
MatchFinder::MatchResult &Result) {
if (const auto *BinOp = Result.Nodes.getNodeAs("binary")) {
// If the expression's constants are macros, check whether they are
// intentional.
+
+//
+// Special case for floating-point representation.
+//
+// If expressions on both sides of comparison operator are of type float,
+// then for some comparison operators no warning shall be
+// reported even if the expressions are identical from a symbolic point of
+// view. Comparison between expressions, declared variables and literals
+// are treated differently.
+//
+// != and == between float literals that have the same value should NOT
+// warn. < > between float literals that have the same value SHOULD warn.
+//
+// != and == between the same float declaration should NOT warn.
+// < > between the same float declaration SHOULD warn.
+//
+// != and == between eq. expressions that evaluates into float
+// should NOT warn.
+// < > between eq. expressions that evaluates into float
+// should NOT warn.
+//
+const Expr *LHS = BinOp->getLHS()->IgnoreParenImpCasts();
+const Expr *RHS = BinOp->getRHS()->IgnoreParenImpCasts();
+BinaryOperator::Opcode Op = BinOp->getOpcode();
+
+const auto *DeclRef1 = dyn_cast(LHS);
+const auto *DeclRef2 = dyn_cast(RHS);
+const auto *FloatLit1 = dyn_cast(LHS);
+const auto *FloatLit2 = dyn_cast(RHS);
+if ((DeclRef1) && (DeclRef2)) {
+ if ((DeclRef1->getType()->hasFloatingRepresentation()) &&
+ (DeclRef2->getType()->hasFloatingRepresentation())) {
+if (DeclRef1->getDecl() == DeclRef2->getDecl()) {
+ if ((Op == BO_EQ) || (Op == BO_NE)) {
+return;
+ }
+}
+ }
+} else if ((FloatLit1) && (FloatLit2)) {
+ if (FloatLit1->getValue().bitwiseIsEqual(FloatLit2->getValue())) {
+if ((Op == BO_EQ) || (Op == BO_NE)) {
+ return;
+}
+ }
5chmidti wrote:
Each of these two blocks of `if` statements could be combined with `&&` instead
of keeping on nesting
https://github.com/llvm/llvm-project/pull/114715
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [analyzer] Remove alpha.core.IdenticalExpr Checker (PR #114715)
@@ -1,4 +1,5 @@ -// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core.IdenticalExpr -w -verify %s +// RUN: clang-tidy %s -checks="-*,misc-redundant-expression" -- 2>&1 | FileCheck %s --check-prefix=IDENTEXPR +// RUN: clang-tidy %s -checks="-*,bugprone-branch-clone" -- 2>&1 | FileCheck %s --check-prefix=BUGPRONEBRANCH 5chmidti wrote: Please use, e.g., ```c++ // RUN: check_clang_tidy %s misc-redundant-expression %t -check-suffix=IDENTEXPR // CHECK-MESSAGES-IDENTEXPR... ``` instead https://github.com/llvm/llvm-project/pull/114715 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [analyzer] Remove alpha.core.IdenticalExpr Checker (PR #114715)
@@ -814,6 +814,12 @@ Improvements Moved checkers ^^ +- The checker ``alpha.core.IdenticalExpr`` was removed because it was + duplicated in the clang-tidy checkers ``misc-redundant-expression`` and 5chmidti wrote: nit https://github.com/llvm/llvm-project/pull/114715 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [analyzer] Remove alpha.core.IdenticalExpr Checker (PR #114715)
https://github.com/5chmidti commented: > some guidance on if the additional "scaffolded" case should be kept in place > (to maintain clear differences from the original to new file in the git > diff), or split the file now or maybe in a future separate commit so the > differences are clear. IMO it is better as a follow-up so that the diff of the commit stays readable, which can also remove duplicate tests. https://github.com/llvm/llvm-project/pull/114715 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [analyzer] Remove alpha.core.IdenticalExpr Checker (PR #114715)
@@ -32,9 +32,28 @@ If this is the intended behavior, then there is no reason to use a conditional statement; otherwise the issue can be solved by fixing the branch that is handled incorrectly. -The check also detects repeated branches in longer ``if/else if/else`` chains +The check detects repeated branches in longer ``if/else if/else`` chains where it would be even harder to notice the problem. +The check also detects repeated inner and outer if statements that may 5chmidti wrote: ``` ``if`` statements ``` https://github.com/llvm/llvm-project/pull/114715 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [analyzer] Remove alpha.core.IdenticalExpr Checker (PR #114715)
5chmidti wrote: Thanks for changing these to be more understandable w.r.t. what has changed. Could you please do a follow-up PR that splits this file, so we have the diff that shows no capabilities were lost in this PR/commit, and then have the tests be where the checks are (e.g., `misc/redundant-expression-csa-identicalexpr.cpp`?) --- FYI: When merging with the other tests in a follow-up, note that clang-tidy has a test-driver for lit, e.g., - `// RUN: %check_clang_tidy %s -check-suffixes=,DEFAULT cppcoreguidelines-avoid-non-const-global-variables %t` for `CHECK-MESSAGES` and `CHECK-MESSAGES-DEFAULT` - `// RUN: %check_clang_tidy %s cppcoreguidelines-avoid-non-const-global-variables %t` for `CHECK-MESSAGES` https://github.com/llvm/llvm-project/pull/114715 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [analyzer] Remove alpha.core.IdenticalExpr Checker (PR #114715)
https://github.com/5chmidti edited https://github.com/llvm/llvm-project/pull/114715 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [analyzer] Remove alpha.core.IdenticalExpr Checker (PR #114715)
https://github.com/vabridgers updated
https://github.com/llvm/llvm-project/pull/114715
>From 3f3df0cf33348912631ca132b53d824fd8d6eb35 Mon Sep 17 00:00:00 2001
From: Vince Bridgers
Date: Thu, 7 Nov 2024 01:58:21 +0100
Subject: [PATCH] [analyzer] Port alpha.core.IdenticalExpr to Tidy checks and
remove
This change removes the alpha.core.IdenticalExpr static analysis
checker since it's checks are present in the clang-tidy checks
misc-redundant-expression and bugprone-branch-clone. This check was
implemented as a static analysis check using AST matching, and since
alpha and duplicated in 2 clang-tidy checks may be removed. The
existing LIT test was checked case by case, and the tidy checks
were improved to maintain alpha.core.IdenticalExpr features.
---
.../clang-tidy/bugprone/BranchCloneCheck.cpp | 220
.../misc/RedundantExpressionCheck.cpp | 76 ++-
clang-tools-extra/docs/ReleaseNotes.rst | 9 +
.../checks/bugprone/branch-clone.rst | 21 +-
.../checks/misc/redundant-expression.rst | 19 +
.../bugprone/alpha-core-identicalexpr.cpp | 335 +++
clang/docs/ReleaseNotes.rst | 6 +
clang/docs/analyzer/checkers.rst | 30 -
.../clang/StaticAnalyzer/Checkers/Checkers.td | 4 -
.../StaticAnalyzer/Checkers/CMakeLists.txt| 1 -
.../Checkers/IdenticalExprChecker.cpp | 520 --
11 files changed, 580 insertions(+), 661 deletions(-)
rename clang/test/Analysis/identical-expressions.cpp =>
clang-tools-extra/test/clang-tidy/checkers/bugprone/alpha-core-identicalexpr.cpp
(60%)
delete mode 100644 clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
diff --git a/clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
b/clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
index 356acf968db921..efd5bce4a32343 100644
--- a/clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
@@ -102,6 +102,209 @@ void BranchCloneCheck::registerMatchers(MatchFinder
*Finder) {
this);
Finder->addMatcher(switchStmt().bind("switch"), this);
Finder->addMatcher(conditionalOperator().bind("condOp"), this);
+
Finder->addMatcher(ifStmt(hasDescendant(ifStmt())).bind("ifWithDescendantIf"),
+ this);
+}
+
+/// Determines whether two statement trees are identical regarding
+/// operators and symbols.
+///
+/// Exceptions: expressions containing macros or functions with possible side
+/// effects are never considered identical.
+/// Limitations: (t + u) and (u + t) are not considered identical.
+/// t*(u + t) and t*u + t*t are not considered identical.
+///
+static bool isIdenticalStmt(const ASTContext &Ctx, const Stmt *Stmt1,
+const Stmt *Stmt2, bool IgnoreSideEffects) {
+
+ if (!Stmt1 || !Stmt2) {
+return !Stmt1 && !Stmt2;
+ }
+
+ // If Stmt1 & Stmt2 are of different class then they are not
+ // identical statements.
+ if (Stmt1->getStmtClass() != Stmt2->getStmtClass())
+return false;
+
+ const auto *Expr1 = dyn_cast(Stmt1);
+ const auto *Expr2 = dyn_cast(Stmt2);
+
+ if (Expr1 && Expr2) {
+// If Stmt1 has side effects then don't warn even if expressions
+// are identical.
+if (!IgnoreSideEffects && Expr1->HasSideEffects(Ctx))
+ return false;
+// If either expression comes from a macro then don't warn even if
+// the expressions are identical.
+if ((Expr1->getExprLoc().isMacroID()) || (Expr2->getExprLoc().isMacroID()))
+ return false;
+
+// If all children of two expressions are identical, return true.
+Expr::const_child_iterator I1 = Expr1->child_begin();
+Expr::const_child_iterator I2 = Expr2->child_begin();
+while (I1 != Expr1->child_end() && I2 != Expr2->child_end()) {
+ if (!*I1 || !*I2 || !isIdenticalStmt(Ctx, *I1, *I2, IgnoreSideEffects))
+return false;
+ ++I1;
+ ++I2;
+}
+// If there are different number of children in the statements, return
+// false.
+if (I1 != Expr1->child_end())
+ return false;
+if (I2 != Expr2->child_end())
+ return false;
+ }
+
+ switch (Stmt1->getStmtClass()) {
+ default:
+return false;
+ case Stmt::CallExprClass:
+ case Stmt::ArraySubscriptExprClass:
+ case Stmt::ArraySectionExprClass:
+ case Stmt::OMPArrayShapingExprClass:
+ case Stmt::OMPIteratorExprClass:
+ case Stmt::ImplicitCastExprClass:
+ case Stmt::ParenExprClass:
+ case Stmt::BreakStmtClass:
+ case Stmt::ContinueStmtClass:
+ case Stmt::NullStmtClass:
+return true;
+ case Stmt::CStyleCastExprClass: {
+const auto *CastExpr1 = cast(Stmt1);
+const auto *CastExpr2 = cast(Stmt2);
+
+return CastExpr1->getTypeAsWritten() == CastExpr2->getTypeAsWritten();
+ }
+ case Stmt::ReturnStmtClass: {
+const auto *ReturnStmt1 = cast(Stmt1);
+const auto *ReturnStmt2 = cast(Stmt2);
+
+return isIdenticalStmt(Ctx, ReturnStmt1->getRetValue(),
+
[clang] [clang-tools-extra] [analyzer] Remove alpha.core.IdenticalExpr Checker (PR #114715)
vabridgers wrote: Thanks @HerrCai0907, I'll update the documents and release notes. Best https://github.com/llvm/llvm-project/pull/114715 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [analyzer] Remove alpha.core.IdenticalExpr Checker (PR #114715)
https://github.com/HerrCai0907 commented: looks good. it extends the checker in clang-tidy so i think documents and releasenote in clang-tidy are also needed https://github.com/llvm/llvm-project/pull/114715 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [analyzer] Remove alpha.core.IdenticalExpr Checker (PR #114715)
https://github.com/vabridgers updated
https://github.com/llvm/llvm-project/pull/114715
>From eed1a2dc946cd642824949f7f00f1e4544264b4a Mon Sep 17 00:00:00 2001
From: Vince Bridgers
Date: Thu, 7 Nov 2024 01:58:21 +0100
Subject: [PATCH] [analyzer] Port alpha.core.IdenticalExpr to Tidy checks and
remove
This change removes the alpha.core.IdenticalExpr static analysis
checker since it's checks are present in the clang-tidy checks
misc-redundant-expression and bugprone-branch-clone. This check was
implemented as a static analysis check using AST matching, and since
alpha and duplicated in 2 clang-tidy checks may be removed. The
existing LIT test was checked case by case, and the tidy checks
were improved to maintain alpha.core.IdenticalExpr features.
---
.../clang-tidy/bugprone/BranchCloneCheck.cpp | 220
.../misc/RedundantExpressionCheck.cpp | 76 ++-
.../bugprone/alpha-core-identicalexpr.cpp | 329 ---
clang/docs/ReleaseNotes.rst | 6 +
clang/docs/analyzer/checkers.rst | 30 -
.../clang/StaticAnalyzer/Checkers/Checkers.td | 4 -
.../StaticAnalyzer/Checkers/CMakeLists.txt| 1 -
.../Checkers/IdenticalExprChecker.cpp | 520 --
8 files changed, 529 insertions(+), 657 deletions(-)
rename clang/test/Analysis/identical-expressions.cpp =>
clang-tools-extra/test/clang-tidy/checkers/bugprone/alpha-core-identicalexpr.cpp
(60%)
delete mode 100644 clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
diff --git a/clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
b/clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
index 356acf968db921..efd5bce4a32343 100644
--- a/clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
@@ -102,6 +102,209 @@ void BranchCloneCheck::registerMatchers(MatchFinder
*Finder) {
this);
Finder->addMatcher(switchStmt().bind("switch"), this);
Finder->addMatcher(conditionalOperator().bind("condOp"), this);
+
Finder->addMatcher(ifStmt(hasDescendant(ifStmt())).bind("ifWithDescendantIf"),
+ this);
+}
+
+/// Determines whether two statement trees are identical regarding
+/// operators and symbols.
+///
+/// Exceptions: expressions containing macros or functions with possible side
+/// effects are never considered identical.
+/// Limitations: (t + u) and (u + t) are not considered identical.
+/// t*(u + t) and t*u + t*t are not considered identical.
+///
+static bool isIdenticalStmt(const ASTContext &Ctx, const Stmt *Stmt1,
+const Stmt *Stmt2, bool IgnoreSideEffects) {
+
+ if (!Stmt1 || !Stmt2) {
+return !Stmt1 && !Stmt2;
+ }
+
+ // If Stmt1 & Stmt2 are of different class then they are not
+ // identical statements.
+ if (Stmt1->getStmtClass() != Stmt2->getStmtClass())
+return false;
+
+ const auto *Expr1 = dyn_cast(Stmt1);
+ const auto *Expr2 = dyn_cast(Stmt2);
+
+ if (Expr1 && Expr2) {
+// If Stmt1 has side effects then don't warn even if expressions
+// are identical.
+if (!IgnoreSideEffects && Expr1->HasSideEffects(Ctx))
+ return false;
+// If either expression comes from a macro then don't warn even if
+// the expressions are identical.
+if ((Expr1->getExprLoc().isMacroID()) || (Expr2->getExprLoc().isMacroID()))
+ return false;
+
+// If all children of two expressions are identical, return true.
+Expr::const_child_iterator I1 = Expr1->child_begin();
+Expr::const_child_iterator I2 = Expr2->child_begin();
+while (I1 != Expr1->child_end() && I2 != Expr2->child_end()) {
+ if (!*I1 || !*I2 || !isIdenticalStmt(Ctx, *I1, *I2, IgnoreSideEffects))
+return false;
+ ++I1;
+ ++I2;
+}
+// If there are different number of children in the statements, return
+// false.
+if (I1 != Expr1->child_end())
+ return false;
+if (I2 != Expr2->child_end())
+ return false;
+ }
+
+ switch (Stmt1->getStmtClass()) {
+ default:
+return false;
+ case Stmt::CallExprClass:
+ case Stmt::ArraySubscriptExprClass:
+ case Stmt::ArraySectionExprClass:
+ case Stmt::OMPArrayShapingExprClass:
+ case Stmt::OMPIteratorExprClass:
+ case Stmt::ImplicitCastExprClass:
+ case Stmt::ParenExprClass:
+ case Stmt::BreakStmtClass:
+ case Stmt::ContinueStmtClass:
+ case Stmt::NullStmtClass:
+return true;
+ case Stmt::CStyleCastExprClass: {
+const auto *CastExpr1 = cast(Stmt1);
+const auto *CastExpr2 = cast(Stmt2);
+
+return CastExpr1->getTypeAsWritten() == CastExpr2->getTypeAsWritten();
+ }
+ case Stmt::ReturnStmtClass: {
+const auto *ReturnStmt1 = cast(Stmt1);
+const auto *ReturnStmt2 = cast(Stmt2);
+
+return isIdenticalStmt(Ctx, ReturnStmt1->getRetValue(),
+ ReturnStmt2->getRetValue(), IgnoreSideEffects);
+ }
+ case Stmt::ForStmtClass: {
+const auto *ForStmt1 = cast(Stmt1);
+const auto *ForStmt2 = ca
[clang] [clang-tools-extra] [analyzer] Remove alpha.core.IdenticalExpr Checker (PR #114715)
@@ -269,6 +472,23 @@ void BranchCloneCheck::check(const
MatchFinder::MatchResult &Result) {
return;
}
+ if (const auto *IS = Result.Nodes.getNodeAs("ifWithDescendantIf")) {
+const Stmt *Then = IS->getThen();
+if (const CompoundStmt *CS = dyn_cast(Then)) {
+ if (!CS->body_empty()) {
+const IfStmt *InnerIf = dyn_cast(*CS->body_begin());
EugeneZelenko wrote:
```suggestion
const auto *InnerIf = dyn_cast(*CS->body_begin());
```
https://github.com/llvm/llvm-project/pull/114715
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [analyzer] Remove alpha.core.IdenticalExpr Checker (PR #114715)
https://github.com/vabridgers updated
https://github.com/llvm/llvm-project/pull/114715
>From 24e27a648944e38447dded20b899bd70fc47212c Mon Sep 17 00:00:00 2001
From: Vince Bridgers
Date: Thu, 7 Nov 2024 01:58:21 +0100
Subject: [PATCH] [analyzer] Port alpha.core.IdenticalExpr to Tidy checks and
remove
This change removes the alpha.core.IdenticalExpr static analysis
checker since it's checks are present in the clang-tidy checks
misc-redundant-expression and bugprone-branch-clone. This check was
implemented as a static analysis check using AST matching, and since
alpha and duplicated in 2 clang-tidy checks may be removed. The
existing LIT test was checked case by case, and the tidy checks
were improved to maintain alpha.core.IdenticalExpr features.
---
.../clang-tidy/bugprone/BranchCloneCheck.cpp | 220
.../misc/RedundantExpressionCheck.cpp | 76 ++-
.../bugprone/alpha-core-identicalexpr.cpp | 329 ---
clang/docs/ReleaseNotes.rst | 6 +
clang/docs/analyzer/checkers.rst | 30 -
.../clang/StaticAnalyzer/Checkers/Checkers.td | 4 -
.../StaticAnalyzer/Checkers/CMakeLists.txt| 1 -
.../Checkers/IdenticalExprChecker.cpp | 520 --
8 files changed, 529 insertions(+), 657 deletions(-)
rename clang/test/Analysis/identical-expressions.cpp =>
clang-tools-extra/test/clang-tidy/checkers/bugprone/alpha-core-identicalexpr.cpp
(60%)
delete mode 100644 clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
diff --git a/clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
b/clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
index 356acf968db921..41475021f1c8f4 100644
--- a/clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
@@ -102,6 +102,209 @@ void BranchCloneCheck::registerMatchers(MatchFinder
*Finder) {
this);
Finder->addMatcher(switchStmt().bind("switch"), this);
Finder->addMatcher(conditionalOperator().bind("condOp"), this);
+
Finder->addMatcher(ifStmt(hasDescendant(ifStmt())).bind("ifWithDescendantIf"),
+ this);
+}
+
+/// Determines whether two statement trees are identical regarding
+/// operators and symbols.
+///
+/// Exceptions: expressions containing macros or functions with possible side
+/// effects are never considered identical.
+/// Limitations: (t + u) and (u + t) are not considered identical.
+/// t*(u + t) and t*u + t*t are not considered identical.
+///
+static bool isIdenticalStmt(const ASTContext &Ctx, const Stmt *Stmt1,
+const Stmt *Stmt2, bool IgnoreSideEffects) {
+
+ if (!Stmt1 || !Stmt2) {
+return !Stmt1 && !Stmt2;
+ }
+
+ // If Stmt1 & Stmt2 are of different class then they are not
+ // identical statements.
+ if (Stmt1->getStmtClass() != Stmt2->getStmtClass())
+return false;
+
+ const auto *Expr1 = dyn_cast(Stmt1);
+ const auto *Expr2 = dyn_cast(Stmt2);
+
+ if (Expr1 && Expr2) {
+// If Stmt1 has side effects then don't warn even if expressions
+// are identical.
+if (!IgnoreSideEffects && Expr1->HasSideEffects(Ctx))
+ return false;
+// If either expression comes from a macro then don't warn even if
+// the expressions are identical.
+if ((Expr1->getExprLoc().isMacroID()) || (Expr2->getExprLoc().isMacroID()))
+ return false;
+
+// If all children of two expressions are identical, return true.
+Expr::const_child_iterator I1 = Expr1->child_begin();
+Expr::const_child_iterator I2 = Expr2->child_begin();
+while (I1 != Expr1->child_end() && I2 != Expr2->child_end()) {
+ if (!*I1 || !*I2 || !isIdenticalStmt(Ctx, *I1, *I2, IgnoreSideEffects))
+return false;
+ ++I1;
+ ++I2;
+}
+// If there are different number of children in the statements, return
+// false.
+if (I1 != Expr1->child_end())
+ return false;
+if (I2 != Expr2->child_end())
+ return false;
+ }
+
+ switch (Stmt1->getStmtClass()) {
+ default:
+return false;
+ case Stmt::CallExprClass:
+ case Stmt::ArraySubscriptExprClass:
+ case Stmt::ArraySectionExprClass:
+ case Stmt::OMPArrayShapingExprClass:
+ case Stmt::OMPIteratorExprClass:
+ case Stmt::ImplicitCastExprClass:
+ case Stmt::ParenExprClass:
+ case Stmt::BreakStmtClass:
+ case Stmt::ContinueStmtClass:
+ case Stmt::NullStmtClass:
+return true;
+ case Stmt::CStyleCastExprClass: {
+const auto *CastExpr1 = cast(Stmt1);
+const auto *CastExpr2 = cast(Stmt2);
+
+return CastExpr1->getTypeAsWritten() == CastExpr2->getTypeAsWritten();
+ }
+ case Stmt::ReturnStmtClass: {
+const auto *ReturnStmt1 = cast(Stmt1);
+const auto *ReturnStmt2 = cast(Stmt2);
+
+return isIdenticalStmt(Ctx, ReturnStmt1->getRetValue(),
+ ReturnStmt2->getRetValue(), IgnoreSideEffects);
+ }
+ case Stmt::ForStmtClass: {
+const auto *ForStmt1 = cast(Stmt1);
+const auto *ForStmt2 = ca
[clang] [clang-tools-extra] [analyzer] Remove alpha.core.IdenticalExpr Checker (PR #114715)
@@ -102,6 +102,209 @@ void BranchCloneCheck::registerMatchers(MatchFinder
*Finder) {
this);
Finder->addMatcher(switchStmt().bind("switch"), this);
Finder->addMatcher(conditionalOperator().bind("condOp"), this);
+
Finder->addMatcher(ifStmt(hasDescendant(ifStmt())).bind("ifWithDescendantIf"),
+ this);
+}
+
+/// Determines whether two statement trees are identical regarding
+/// operators and symbols.
+///
+/// Exceptions: expressions containing macros or functions with possible side
+/// effects are never considered identical.
+/// Limitations: (t + u) and (u + t) are not considered identical.
+/// t*(u + t) and t*u + t*t are not considered identical.
+///
+static bool isIdenticalStmt(const ASTContext &Ctx, const Stmt *Stmt1,
+const Stmt *Stmt2, bool IgnoreSideEffects) {
+
+ if (!Stmt1 || !Stmt2) {
+return !Stmt1 && !Stmt2;
+ }
+
+ // If Stmt1 & Stmt2 are of different class then they are not
+ // identical statements.
+ if (Stmt1->getStmtClass() != Stmt2->getStmtClass())
+return false;
+
+ const auto *Expr1 = dyn_cast(Stmt1);
+ const auto *Expr2 = dyn_cast(Stmt2);
+
+ if (Expr1 && Expr2) {
+// If Stmt1 has side effects then don't warn even if expressions
+// are identical.
+if (!IgnoreSideEffects && Expr1->HasSideEffects(Ctx))
+ return false;
+// If either expression comes from a macro then don't warn even if
+// the expressions are identical.
+if ((Expr1->getExprLoc().isMacroID()) || (Expr2->getExprLoc().isMacroID()))
+ return false;
+
+// If all children of two expressions are identical, return true.
+Expr::const_child_iterator I1 = Expr1->child_begin();
+Expr::const_child_iterator I2 = Expr2->child_begin();
+while (I1 != Expr1->child_end() && I2 != Expr2->child_end()) {
+ if (!*I1 || !*I2 || !isIdenticalStmt(Ctx, *I1, *I2, IgnoreSideEffects))
+return false;
+ ++I1;
+ ++I2;
+}
+// If there are different number of children in the statements, return
+// false.
+if (I1 != Expr1->child_end())
+ return false;
+if (I2 != Expr2->child_end())
+ return false;
+ }
+
+ switch (Stmt1->getStmtClass()) {
+ default:
+return false;
+ case Stmt::CallExprClass:
+ case Stmt::ArraySubscriptExprClass:
+ case Stmt::ArraySectionExprClass:
+ case Stmt::OMPArrayShapingExprClass:
+ case Stmt::OMPIteratorExprClass:
+ case Stmt::ImplicitCastExprClass:
+ case Stmt::ParenExprClass:
+ case Stmt::BreakStmtClass:
+ case Stmt::ContinueStmtClass:
+ case Stmt::NullStmtClass:
+return true;
+ case Stmt::CStyleCastExprClass: {
+const CStyleCastExpr *CastExpr1 = cast(Stmt1);
EugeneZelenko wrote:
```suggestion
const auto *CastExpr1 = cast(Stmt1);
```
Same in many other similar statements below.
https://github.com/llvm/llvm-project/pull/114715
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [analyzer] Remove alpha.core.IdenticalExpr Checker (PR #114715)
https://github.com/vabridgers updated
https://github.com/llvm/llvm-project/pull/114715
>From d20240f0c38a33707ba773a0f49081f076e5421b Mon Sep 17 00:00:00 2001
From: Vince Bridgers
Date: Thu, 7 Nov 2024 01:58:21 +0100
Subject: [PATCH] [analyzer] Port alpha.core.IdenticalExpr to Tidy checks and
remove
This change removes the alpha.core.IdenticalExpr static analysis
checker since it's checks are present in the clang-tidy checks
misc-redundant-expression and bugprone-branch-clone. This check was
implemented as a static analysis check using AST matching, and since
alpha and duplicated in 2 clang-tidy checks may be removed. The
existing LIT test was checked case by case, and the tidy checks
were improved to maintain alpha.core.IdenticalExpr features.
---
.../clang-tidy/bugprone/BranchCloneCheck.cpp | 220
.../misc/RedundantExpressionCheck.cpp | 76 ++-
.../bugprone/alpha-core-identicalexpr.cpp | 329 ---
clang/docs/ReleaseNotes.rst | 6 +
clang/docs/analyzer/checkers.rst | 30 -
.../clang/StaticAnalyzer/Checkers/Checkers.td | 4 -
.../StaticAnalyzer/Checkers/CMakeLists.txt| 1 -
.../Checkers/IdenticalExprChecker.cpp | 520 --
8 files changed, 529 insertions(+), 657 deletions(-)
rename clang/test/Analysis/identical-expressions.cpp =>
clang-tools-extra/test/clang-tidy/checkers/bugprone/alpha-core-identicalexpr.cpp
(60%)
delete mode 100644 clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
diff --git a/clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
b/clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
index 356acf968db921..dc7ff50194f04c 100644
--- a/clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
@@ -102,6 +102,209 @@ void BranchCloneCheck::registerMatchers(MatchFinder
*Finder) {
this);
Finder->addMatcher(switchStmt().bind("switch"), this);
Finder->addMatcher(conditionalOperator().bind("condOp"), this);
+
Finder->addMatcher(ifStmt(hasDescendant(ifStmt())).bind("ifWithDescendantIf"),
+ this);
+}
+
+/// Determines whether two statement trees are identical regarding
+/// operators and symbols.
+///
+/// Exceptions: expressions containing macros or functions with possible side
+/// effects are never considered identical.
+/// Limitations: (t + u) and (u + t) are not considered identical.
+/// t*(u + t) and t*u + t*t are not considered identical.
+///
+static bool isIdenticalStmt(const ASTContext &Ctx, const Stmt *Stmt1,
+const Stmt *Stmt2, bool IgnoreSideEffects) {
+
+ if (!Stmt1 || !Stmt2) {
+return !Stmt1 && !Stmt2;
+ }
+
+ // If Stmt1 & Stmt2 are of different class then they are not
+ // identical statements.
+ if (Stmt1->getStmtClass() != Stmt2->getStmtClass())
+return false;
+
+ const auto *Expr1 = dyn_cast(Stmt1);
+ const auto *Expr2 = dyn_cast(Stmt2);
+
+ if (Expr1 && Expr2) {
+// If Stmt1 has side effects then don't warn even if expressions
+// are identical.
+if (!IgnoreSideEffects && Expr1->HasSideEffects(Ctx))
+ return false;
+// If either expression comes from a macro then don't warn even if
+// the expressions are identical.
+if ((Expr1->getExprLoc().isMacroID()) || (Expr2->getExprLoc().isMacroID()))
+ return false;
+
+// If all children of two expressions are identical, return true.
+Expr::const_child_iterator I1 = Expr1->child_begin();
+Expr::const_child_iterator I2 = Expr2->child_begin();
+while (I1 != Expr1->child_end() && I2 != Expr2->child_end()) {
+ if (!*I1 || !*I2 || !isIdenticalStmt(Ctx, *I1, *I2, IgnoreSideEffects))
+return false;
+ ++I1;
+ ++I2;
+}
+// If there are different number of children in the statements, return
+// false.
+if (I1 != Expr1->child_end())
+ return false;
+if (I2 != Expr2->child_end())
+ return false;
+ }
+
+ switch (Stmt1->getStmtClass()) {
+ default:
+return false;
+ case Stmt::CallExprClass:
+ case Stmt::ArraySubscriptExprClass:
+ case Stmt::ArraySectionExprClass:
+ case Stmt::OMPArrayShapingExprClass:
+ case Stmt::OMPIteratorExprClass:
+ case Stmt::ImplicitCastExprClass:
+ case Stmt::ParenExprClass:
+ case Stmt::BreakStmtClass:
+ case Stmt::ContinueStmtClass:
+ case Stmt::NullStmtClass:
+return true;
+ case Stmt::CStyleCastExprClass: {
+const CStyleCastExpr *CastExpr1 = cast(Stmt1);
+const CStyleCastExpr *CastExpr2 = cast(Stmt2);
+
+return CastExpr1->getTypeAsWritten() == CastExpr2->getTypeAsWritten();
+ }
+ case Stmt::ReturnStmtClass: {
+const auto *ReturnStmt1 = cast(Stmt1);
+const auto *ReturnStmt2 = cast(Stmt2);
+
+return isIdenticalStmt(Ctx, ReturnStmt1->getRetValue(),
+ ReturnStmt2->getRetValue(), IgnoreSideEffects);
+ }
+ case Stmt::ForStmtClass: {
+const auto *ForStmt1 = cast(Stmt1);
+const
[clang] [clang-tools-extra] [analyzer] Remove alpha.core.IdenticalExpr Checker (PR #114715)
@@ -102,6 +102,209 @@ void BranchCloneCheck::registerMatchers(MatchFinder
*Finder) {
this);
Finder->addMatcher(switchStmt().bind("switch"), this);
Finder->addMatcher(conditionalOperator().bind("condOp"), this);
+
Finder->addMatcher(ifStmt(hasDescendant(ifStmt())).bind("ifWithDescendantIf"),
+ this);
+}
+
+/// Determines whether two statement trees are identical regarding
+/// operators and symbols.
+///
+/// Exceptions: expressions containing macros or functions with possible side
+/// effects are never considered identical.
+/// Limitations: (t + u) and (u + t) are not considered identical.
+/// t*(u + t) and t*u + t*t are not considered identical.
+///
+static bool isIdenticalStmt(const ASTContext &Ctx, const Stmt *Stmt1,
+const Stmt *Stmt2, bool IgnoreSideEffects) {
+
+ if (!Stmt1 || !Stmt2) {
+return !Stmt1 && !Stmt2;
+ }
+
+ // If Stmt1 & Stmt2 are of different class then they are not
+ // identical statements.
+ if (Stmt1->getStmtClass() != Stmt2->getStmtClass())
+return false;
+
+ const Expr *Expr1 = dyn_cast(Stmt1);
+ const Expr *Expr2 = dyn_cast(Stmt2);
+
+ if (Expr1 && Expr2) {
+// If Stmt1 has side effects then don't warn even if expressions
+// are identical.
+if (!IgnoreSideEffects && Expr1->HasSideEffects(Ctx))
+ return false;
+// If either expression comes from a macro then don't warn even if
+// the expressions are identical.
+if ((Expr1->getExprLoc().isMacroID()) || (Expr2->getExprLoc().isMacroID()))
+ return false;
+
+// If all children of two expressions are identical, return true.
+Expr::const_child_iterator I1 = Expr1->child_begin();
+Expr::const_child_iterator I2 = Expr2->child_begin();
+while (I1 != Expr1->child_end() && I2 != Expr2->child_end()) {
+ if (!*I1 || !*I2 || !isIdenticalStmt(Ctx, *I1, *I2, IgnoreSideEffects))
+return false;
+ ++I1;
+ ++I2;
+}
+// If there are different number of children in the statements, return
+// false.
+if (I1 != Expr1->child_end())
+ return false;
+if (I2 != Expr2->child_end())
+ return false;
+ }
+
+ switch (Stmt1->getStmtClass()) {
+ default:
+return false;
+ case Stmt::CallExprClass:
+ case Stmt::ArraySubscriptExprClass:
+ case Stmt::ArraySectionExprClass:
+ case Stmt::OMPArrayShapingExprClass:
+ case Stmt::OMPIteratorExprClass:
+ case Stmt::ImplicitCastExprClass:
+ case Stmt::ParenExprClass:
+ case Stmt::BreakStmtClass:
+ case Stmt::ContinueStmtClass:
+ case Stmt::NullStmtClass:
+return true;
+ case Stmt::CStyleCastExprClass: {
+const CStyleCastExpr *CastExpr1 = cast(Stmt1);
+const CStyleCastExpr *CastExpr2 = cast(Stmt2);
+
+return CastExpr1->getTypeAsWritten() == CastExpr2->getTypeAsWritten();
+ }
+ case Stmt::ReturnStmtClass: {
+const ReturnStmt *ReturnStmt1 = cast(Stmt1);
+const ReturnStmt *ReturnStmt2 = cast(Stmt2);
EugeneZelenko wrote:
```suggestion
const auto *ReturnStmt2 = cast(Stmt2);
```
Same for other similar statements.
https://github.com/llvm/llvm-project/pull/114715
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [analyzer] Remove alpha.core.IdenticalExpr Checker (PR #114715)
@@ -102,6 +102,209 @@ void BranchCloneCheck::registerMatchers(MatchFinder
*Finder) {
this);
Finder->addMatcher(switchStmt().bind("switch"), this);
Finder->addMatcher(conditionalOperator().bind("condOp"), this);
+
Finder->addMatcher(ifStmt(hasDescendant(ifStmt())).bind("ifWithDescendantIf"),
+ this);
+}
+
+/// Determines whether two statement trees are identical regarding
+/// operators and symbols.
+///
+/// Exceptions: expressions containing macros or functions with possible side
+/// effects are never considered identical.
+/// Limitations: (t + u) and (u + t) are not considered identical.
+/// t*(u + t) and t*u + t*t are not considered identical.
+///
+static bool isIdenticalStmt(const ASTContext &Ctx, const Stmt *Stmt1,
+const Stmt *Stmt2, bool IgnoreSideEffects) {
+
+ if (!Stmt1 || !Stmt2) {
+return !Stmt1 && !Stmt2;
+ }
+
+ // If Stmt1 & Stmt2 are of different class then they are not
+ // identical statements.
+ if (Stmt1->getStmtClass() != Stmt2->getStmtClass())
+return false;
+
+ const Expr *Expr1 = dyn_cast(Stmt1);
+ const Expr *Expr2 = dyn_cast(Stmt2);
+
+ if (Expr1 && Expr2) {
+// If Stmt1 has side effects then don't warn even if expressions
+// are identical.
+if (!IgnoreSideEffects && Expr1->HasSideEffects(Ctx))
+ return false;
+// If either expression comes from a macro then don't warn even if
+// the expressions are identical.
+if ((Expr1->getExprLoc().isMacroID()) || (Expr2->getExprLoc().isMacroID()))
+ return false;
+
+// If all children of two expressions are identical, return true.
+Expr::const_child_iterator I1 = Expr1->child_begin();
+Expr::const_child_iterator I2 = Expr2->child_begin();
+while (I1 != Expr1->child_end() && I2 != Expr2->child_end()) {
+ if (!*I1 || !*I2 || !isIdenticalStmt(Ctx, *I1, *I2, IgnoreSideEffects))
+return false;
+ ++I1;
+ ++I2;
+}
+// If there are different number of children in the statements, return
+// false.
+if (I1 != Expr1->child_end())
+ return false;
+if (I2 != Expr2->child_end())
+ return false;
+ }
+
+ switch (Stmt1->getStmtClass()) {
+ default:
+return false;
+ case Stmt::CallExprClass:
+ case Stmt::ArraySubscriptExprClass:
+ case Stmt::ArraySectionExprClass:
+ case Stmt::OMPArrayShapingExprClass:
+ case Stmt::OMPIteratorExprClass:
+ case Stmt::ImplicitCastExprClass:
+ case Stmt::ParenExprClass:
+ case Stmt::BreakStmtClass:
+ case Stmt::ContinueStmtClass:
+ case Stmt::NullStmtClass:
+return true;
+ case Stmt::CStyleCastExprClass: {
+const CStyleCastExpr *CastExpr1 = cast(Stmt1);
+const CStyleCastExpr *CastExpr2 = cast(Stmt2);
+
+return CastExpr1->getTypeAsWritten() == CastExpr2->getTypeAsWritten();
+ }
+ case Stmt::ReturnStmtClass: {
+const ReturnStmt *ReturnStmt1 = cast(Stmt1);
EugeneZelenko wrote:
```suggestion
const auto *ReturnStmt1 = cast(Stmt1);
```
https://github.com/llvm/llvm-project/pull/114715
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [analyzer] Remove alpha.core.IdenticalExpr Checker (PR #114715)
@@ -1238,6 +1235,59 @@ void RedundantExpressionCheck::check(const
MatchFinder::MatchResult &Result) {
if (const auto *BinOp = Result.Nodes.getNodeAs("binary")) {
// If the expression's constants are macros, check whether they are
// intentional.
+
+//
+// Special case for floating-point representation.
+//
+// If expressions on both sides of comparison operator are of type float,
+// then for some comparison operators no warning shall be
+// reported even if the expressions are identical from a symbolic point of
+// view. Comparison between expressions, declared variables and literals
+// are treated differently.
+//
+// != and == between float literals that have the same value should NOT
+// warn. < > between float literals that have the same value SHOULD warn.
+//
+// != and == between the same float declaration should NOT warn.
+// < > between the same float declaration SHOULD warn.
+//
+// != and == between eq. expressions that evaluates into float
+// should NOT warn.
+// < > between eq. expressions that evaluates into float
+// should NOT warn.
+//
+const Expr *LHS = BinOp->getLHS()->IgnoreParenImpCasts();
+const Expr *RHS = BinOp->getRHS()->IgnoreParenImpCasts();
+BinaryOperator::Opcode Op = BinOp->getOpcode();
+
+const DeclRefExpr *DeclRef1 = dyn_cast(LHS);
EugeneZelenko wrote:
```suggestion
const auto *DeclRef1 = dyn_cast(LHS);
```
Same below.
https://github.com/llvm/llvm-project/pull/114715
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [analyzer] Remove alpha.core.IdenticalExpr Checker (PR #114715)
@@ -102,6 +102,209 @@ void BranchCloneCheck::registerMatchers(MatchFinder
*Finder) {
this);
Finder->addMatcher(switchStmt().bind("switch"), this);
Finder->addMatcher(conditionalOperator().bind("condOp"), this);
+
Finder->addMatcher(ifStmt(hasDescendant(ifStmt())).bind("ifWithDescendantIf"),
+ this);
+}
+
+/// Determines whether two statement trees are identical regarding
+/// operators and symbols.
+///
+/// Exceptions: expressions containing macros or functions with possible side
+/// effects are never considered identical.
+/// Limitations: (t + u) and (u + t) are not considered identical.
+/// t*(u + t) and t*u + t*t are not considered identical.
+///
+static bool isIdenticalStmt(const ASTContext &Ctx, const Stmt *Stmt1,
+const Stmt *Stmt2, bool IgnoreSideEffects) {
+
+ if (!Stmt1 || !Stmt2) {
+return !Stmt1 && !Stmt2;
+ }
+
+ // If Stmt1 & Stmt2 are of different class then they are not
+ // identical statements.
+ if (Stmt1->getStmtClass() != Stmt2->getStmtClass())
+return false;
+
+ const Expr *Expr1 = dyn_cast(Stmt1);
+ const Expr *Expr2 = dyn_cast(Stmt2);
EugeneZelenko wrote:
```suggestion
const auto *Expr2 = dyn_cast(Stmt2);
```
https://github.com/llvm/llvm-project/pull/114715
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [analyzer] Remove alpha.core.IdenticalExpr Checker (PR #114715)
@@ -102,6 +102,209 @@ void BranchCloneCheck::registerMatchers(MatchFinder
*Finder) {
this);
Finder->addMatcher(switchStmt().bind("switch"), this);
Finder->addMatcher(conditionalOperator().bind("condOp"), this);
+
Finder->addMatcher(ifStmt(hasDescendant(ifStmt())).bind("ifWithDescendantIf"),
+ this);
+}
+
+/// Determines whether two statement trees are identical regarding
+/// operators and symbols.
+///
+/// Exceptions: expressions containing macros or functions with possible side
+/// effects are never considered identical.
+/// Limitations: (t + u) and (u + t) are not considered identical.
+/// t*(u + t) and t*u + t*t are not considered identical.
+///
+static bool isIdenticalStmt(const ASTContext &Ctx, const Stmt *Stmt1,
+const Stmt *Stmt2, bool IgnoreSideEffects) {
+
+ if (!Stmt1 || !Stmt2) {
+return !Stmt1 && !Stmt2;
+ }
+
+ // If Stmt1 & Stmt2 are of different class then they are not
+ // identical statements.
+ if (Stmt1->getStmtClass() != Stmt2->getStmtClass())
+return false;
+
+ const Expr *Expr1 = dyn_cast(Stmt1);
EugeneZelenko wrote:
```suggestion
const auto *Expr1 = dyn_cast(Stmt1);
```
Type is explicitly stated in same statement.
https://github.com/llvm/llvm-project/pull/114715
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [analyzer] Remove alpha.core.IdenticalExpr Checker (PR #114715)
https://github.com/vabridgers updated
https://github.com/llvm/llvm-project/pull/114715
>From 2834ec4708bfd54bafd28e5d55f5610bcd8593c8 Mon Sep 17 00:00:00 2001
From: Vince Bridgers
Date: Thu, 7 Nov 2024 01:58:21 +0100
Subject: [PATCH] [analyzer] Port alpha.core.IdenticalExpr to Tidy checks and
remove
This change removes the alpha.core.IdenticalExpr static analysis
checker since it's checks are present in the clang-tidy checks
misc-redundant-expression and bugprone-branch-clone. This check was
implemented as a static analysis check using AST matching, and since
alpha and duplicated in 2 clang-tidy checks may be removed. The
existing LIT test was checked case by case, and the tidy checks
were improved to maintain alpha.core.IdenticalExpr features.
---
.../clang-tidy/bugprone/BranchCloneCheck.cpp | 220
.../misc/RedundantExpressionCheck.cpp | 76 ++-
.../bugprone/alpha-core-identicalexpr.cpp | 329 ---
clang/docs/ReleaseNotes.rst | 6 +
clang/docs/analyzer/checkers.rst | 30 -
.../clang/StaticAnalyzer/Checkers/Checkers.td | 4 -
.../StaticAnalyzer/Checkers/CMakeLists.txt| 1 -
.../Checkers/IdenticalExprChecker.cpp | 520 --
8 files changed, 529 insertions(+), 657 deletions(-)
rename clang/test/Analysis/identical-expressions.cpp =>
clang-tools-extra/test/clang-tidy/checkers/bugprone/alpha-core-identicalexpr.cpp
(60%)
delete mode 100644 clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
diff --git a/clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
b/clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
index 356acf968db921..83f181dedec57e 100644
--- a/clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
@@ -102,6 +102,209 @@ void BranchCloneCheck::registerMatchers(MatchFinder
*Finder) {
this);
Finder->addMatcher(switchStmt().bind("switch"), this);
Finder->addMatcher(conditionalOperator().bind("condOp"), this);
+
Finder->addMatcher(ifStmt(hasDescendant(ifStmt())).bind("ifWithDescendantIf"),
+ this);
+}
+
+/// Determines whether two statement trees are identical regarding
+/// operators and symbols.
+///
+/// Exceptions: expressions containing macros or functions with possible side
+/// effects are never considered identical.
+/// Limitations: (t + u) and (u + t) are not considered identical.
+/// t*(u + t) and t*u + t*t are not considered identical.
+///
+static bool isIdenticalStmt(const ASTContext &Ctx, const Stmt *Stmt1,
+const Stmt *Stmt2, bool IgnoreSideEffects) {
+
+ if (!Stmt1 || !Stmt2) {
+return !Stmt1 && !Stmt2;
+ }
+
+ // If Stmt1 & Stmt2 are of different class then they are not
+ // identical statements.
+ if (Stmt1->getStmtClass() != Stmt2->getStmtClass())
+return false;
+
+ const Expr *Expr1 = dyn_cast(Stmt1);
+ const Expr *Expr2 = dyn_cast(Stmt2);
+
+ if (Expr1 && Expr2) {
+// If Stmt1 has side effects then don't warn even if expressions
+// are identical.
+if (!IgnoreSideEffects && Expr1->HasSideEffects(Ctx))
+ return false;
+// If either expression comes from a macro then don't warn even if
+// the expressions are identical.
+if ((Expr1->getExprLoc().isMacroID()) || (Expr2->getExprLoc().isMacroID()))
+ return false;
+
+// If all children of two expressions are identical, return true.
+Expr::const_child_iterator I1 = Expr1->child_begin();
+Expr::const_child_iterator I2 = Expr2->child_begin();
+while (I1 != Expr1->child_end() && I2 != Expr2->child_end()) {
+ if (!*I1 || !*I2 || !isIdenticalStmt(Ctx, *I1, *I2, IgnoreSideEffects))
+return false;
+ ++I1;
+ ++I2;
+}
+// If there are different number of children in the statements, return
+// false.
+if (I1 != Expr1->child_end())
+ return false;
+if (I2 != Expr2->child_end())
+ return false;
+ }
+
+ switch (Stmt1->getStmtClass()) {
+ default:
+return false;
+ case Stmt::CallExprClass:
+ case Stmt::ArraySubscriptExprClass:
+ case Stmt::ArraySectionExprClass:
+ case Stmt::OMPArrayShapingExprClass:
+ case Stmt::OMPIteratorExprClass:
+ case Stmt::ImplicitCastExprClass:
+ case Stmt::ParenExprClass:
+ case Stmt::BreakStmtClass:
+ case Stmt::ContinueStmtClass:
+ case Stmt::NullStmtClass:
+return true;
+ case Stmt::CStyleCastExprClass: {
+const CStyleCastExpr *CastExpr1 = cast(Stmt1);
+const CStyleCastExpr *CastExpr2 = cast(Stmt2);
+
+return CastExpr1->getTypeAsWritten() == CastExpr2->getTypeAsWritten();
+ }
+ case Stmt::ReturnStmtClass: {
+const ReturnStmt *ReturnStmt1 = cast(Stmt1);
+const ReturnStmt *ReturnStmt2 = cast(Stmt2);
+
+return isIdenticalStmt(Ctx, ReturnStmt1->getRetValue(),
+ ReturnStmt2->getRetValue(), IgnoreSideEffects);
+ }
+ case Stmt::ForStmtClass: {
+const ForStmt *ForStmt1 = cast(Stm
[clang] [clang-tools-extra] [analyzer] Remove alpha.core.IdenticalExpr Checker (PR #114715)
vabridgers wrote: I've reworked the test port in a "scaffolded" way so reviewers may see how the cases were considered one by one, and the existing tidy checkers improved to address cases IdenticalExpr was detecting and the tidy checkers were not. I needed to weaken the misc-redundant-expression AST matcher a bit, and consider an extra case for the bugprone-branch-clone checker. This picked up the integer and floating point literal cases, and also the duplicate outer/inner if cases. I'm happy to take next suggested steps to drive this improvement to completion, and would like some guidance on if the additional "scaffolded" case should be kept in place (to maintain clear differences from the original to new file in the git diff), or split the file now or maybe in a future separate commit so the differences are clear. Thanks - Vince https://github.com/llvm/llvm-project/pull/114715 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [analyzer] Remove alpha.core.IdenticalExpr Checker (PR #114715)
github-actions[bot] wrote:
:warning: C/C++ code formatter, clang-format found issues in your code.
:warning:
You can test this locally with the following command:
``bash
git-clang-format --diff 0daca808ce111f21db8c0ee9ea5d2509d6034557
8f80764dd43013d9f6fbd1bb45642a1bfa4eab9e --extensions cpp --
clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
clang-tools-extra/test/clang-tidy/checkers/bugprone/alpha-core-identicalexpr.cpp
``
View the diff from clang-format here.
``diff
diff --git a/clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
b/clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
index bb8418ba43..cab7a20105 100644
--- a/clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
+++ b/clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
@@ -865,20 +865,18 @@ void
RedundantExpressionCheck::registerMatchers(MatchFinder *Finder) {
// Binary with equivalent operands, like (X != 2 && X != 2).
Finder->addMatcher(
- traverse(
- TK_AsIs,
- binaryOperator(
- anyOf(isComparisonOperator(),
-hasAnyOperatorName("-", "/", "%", "|", "&", "^", "&&",
"||",
- "=")),
- operandsAreEquivalent(),
- // Filter noisy false positives.
- unless(isInTemplateInstantiation()),
- unless(binaryOperatorIsInMacro()),
- unless(hasAncestor(arraySubscriptExpr())),
- unless(hasDescendant(BannedIntegerLiteral)),
- unless(IsInUnevaluatedContext))
- .bind("binary")),
+ traverse(TK_AsIs,
+ binaryOperator(anyOf(isComparisonOperator(),
+hasAnyOperatorName("-", "/", "%", "|", "&",
+ "^", "&&", "||", "=")),
+ operandsAreEquivalent(),
+ // Filter noisy false positives.
+ unless(isInTemplateInstantiation()),
+ unless(binaryOperatorIsInMacro()),
+ unless(hasAncestor(arraySubscriptExpr())),
+ unless(hasDescendant(BannedIntegerLiteral)),
+ unless(IsInUnevaluatedContext))
+ .bind("binary")),
this);
// Logical or bitwise operator with equivalent nested operands, like (X && Y
``
https://github.com/llvm/llvm-project/pull/114715
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [analyzer] Remove alpha.core.IdenticalExpr Checker (PR #114715)
https://github.com/vabridgers updated
https://github.com/llvm/llvm-project/pull/114715
>From 8f80764dd43013d9f6fbd1bb45642a1bfa4eab9e Mon Sep 17 00:00:00 2001
From: Vince Bridgers
Date: Thu, 7 Nov 2024 01:58:21 +0100
Subject: [PATCH] [analyzer] Port alpha.core.IdenticalExpr to Tidy checks and
remove
This change removes the alpha.core.IdenticalExpr static analysis
checker since it's checks are present in the clang-tidy checks
misc-redundant-expression and bugprone-branch-clone. This check was
implemented as a static analysis check using AST matching, and since
alpha and duplicated in 2 clang-tidy checks may be removed. The
existing LIT test was checked case by case, and the tidy checks
were improved to maintain alpha.core.IdenticalExpr features.
---
.../clang-tidy/bugprone/BranchCloneCheck.cpp | 220
.../misc/RedundantExpressionCheck.cpp | 82 ++-
.../bugprone/alpha-core-identicalexpr.cpp | 329 ---
clang/docs/ReleaseNotes.rst | 6 +
clang/docs/analyzer/checkers.rst | 30 -
.../clang/StaticAnalyzer/Checkers/Checkers.td | 4 -
.../StaticAnalyzer/Checkers/CMakeLists.txt| 1 -
.../Checkers/IdenticalExprChecker.cpp | 520 --
8 files changed, 533 insertions(+), 659 deletions(-)
rename clang/test/Analysis/identical-expressions.cpp =>
clang-tools-extra/test/clang-tidy/checkers/bugprone/alpha-core-identicalexpr.cpp
(60%)
delete mode 100644 clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
diff --git a/clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
b/clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
index 356acf968db921..83f181dedec57e 100644
--- a/clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
@@ -102,6 +102,209 @@ void BranchCloneCheck::registerMatchers(MatchFinder
*Finder) {
this);
Finder->addMatcher(switchStmt().bind("switch"), this);
Finder->addMatcher(conditionalOperator().bind("condOp"), this);
+
Finder->addMatcher(ifStmt(hasDescendant(ifStmt())).bind("ifWithDescendantIf"),
+ this);
+}
+
+/// Determines whether two statement trees are identical regarding
+/// operators and symbols.
+///
+/// Exceptions: expressions containing macros or functions with possible side
+/// effects are never considered identical.
+/// Limitations: (t + u) and (u + t) are not considered identical.
+/// t*(u + t) and t*u + t*t are not considered identical.
+///
+static bool isIdenticalStmt(const ASTContext &Ctx, const Stmt *Stmt1,
+const Stmt *Stmt2, bool IgnoreSideEffects) {
+
+ if (!Stmt1 || !Stmt2) {
+return !Stmt1 && !Stmt2;
+ }
+
+ // If Stmt1 & Stmt2 are of different class then they are not
+ // identical statements.
+ if (Stmt1->getStmtClass() != Stmt2->getStmtClass())
+return false;
+
+ const Expr *Expr1 = dyn_cast(Stmt1);
+ const Expr *Expr2 = dyn_cast(Stmt2);
+
+ if (Expr1 && Expr2) {
+// If Stmt1 has side effects then don't warn even if expressions
+// are identical.
+if (!IgnoreSideEffects && Expr1->HasSideEffects(Ctx))
+ return false;
+// If either expression comes from a macro then don't warn even if
+// the expressions are identical.
+if ((Expr1->getExprLoc().isMacroID()) || (Expr2->getExprLoc().isMacroID()))
+ return false;
+
+// If all children of two expressions are identical, return true.
+Expr::const_child_iterator I1 = Expr1->child_begin();
+Expr::const_child_iterator I2 = Expr2->child_begin();
+while (I1 != Expr1->child_end() && I2 != Expr2->child_end()) {
+ if (!*I1 || !*I2 || !isIdenticalStmt(Ctx, *I1, *I2, IgnoreSideEffects))
+return false;
+ ++I1;
+ ++I2;
+}
+// If there are different number of children in the statements, return
+// false.
+if (I1 != Expr1->child_end())
+ return false;
+if (I2 != Expr2->child_end())
+ return false;
+ }
+
+ switch (Stmt1->getStmtClass()) {
+ default:
+return false;
+ case Stmt::CallExprClass:
+ case Stmt::ArraySubscriptExprClass:
+ case Stmt::ArraySectionExprClass:
+ case Stmt::OMPArrayShapingExprClass:
+ case Stmt::OMPIteratorExprClass:
+ case Stmt::ImplicitCastExprClass:
+ case Stmt::ParenExprClass:
+ case Stmt::BreakStmtClass:
+ case Stmt::ContinueStmtClass:
+ case Stmt::NullStmtClass:
+return true;
+ case Stmt::CStyleCastExprClass: {
+const CStyleCastExpr *CastExpr1 = cast(Stmt1);
+const CStyleCastExpr *CastExpr2 = cast(Stmt2);
+
+return CastExpr1->getTypeAsWritten() == CastExpr2->getTypeAsWritten();
+ }
+ case Stmt::ReturnStmtClass: {
+const ReturnStmt *ReturnStmt1 = cast(Stmt1);
+const ReturnStmt *ReturnStmt2 = cast(Stmt2);
+
+return isIdenticalStmt(Ctx, ReturnStmt1->getRetValue(),
+ ReturnStmt2->getRetValue(), IgnoreSideEffects);
+ }
+ case Stmt::ForStmtClass: {
+const ForStmt *ForStmt1 = cast(Stm
[clang] [clang-tools-extra] [analyzer] Remove alpha.core.IdenticalExpr Checker (PR #114715)
https://github.com/vabridgers updated
https://github.com/llvm/llvm-project/pull/114715
>From 2508ede979f4061fd752d64967096fee1f4a6127 Mon Sep 17 00:00:00 2001
From: Vince Bridgers
Date: Thu, 7 Nov 2024 01:58:21 +0100
Subject: [PATCH] [analyzer] Port alpha.core.IdenticalExpr to Tidy checks and
remove
This change removes the alpha.core.IdenticalExpr static analysis
checker since it's checks are present in the clang-tidy checks
misc-redundant-expression and bugprone-branch-clone. This check was
implemented as a static analysis check using AST matching, and since
alpha and duplicated in 2 clang-tidy checks may be removed. The
existing LIT test was checked case by case, and the tidy checks
were improved to maintain alpha.core.IdenticalExpr features.
---
.../clang-tidy/bugprone/BranchCloneCheck.cpp | 220
.../misc/RedundantExpressionCheck.cpp | 86 ++-
.../bugprone/alpha-core-identicalexpr.cpp | 329 ---
clang/docs/ReleaseNotes.rst | 6 +
clang/docs/analyzer/checkers.rst | 30 -
.../clang/StaticAnalyzer/Checkers/Checkers.td | 4 -
.../StaticAnalyzer/Checkers/CMakeLists.txt| 1 -
.../Checkers/IdenticalExprChecker.cpp | 520 --
8 files changed, 537 insertions(+), 659 deletions(-)
rename clang/test/Analysis/identical-expressions.cpp =>
clang-tools-extra/test/clang-tidy/checkers/bugprone/alpha-core-identicalexpr.cpp
(60%)
delete mode 100644 clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
diff --git a/clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
b/clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
index 356acf968db921..83f181dedec57e 100644
--- a/clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
@@ -102,6 +102,209 @@ void BranchCloneCheck::registerMatchers(MatchFinder
*Finder) {
this);
Finder->addMatcher(switchStmt().bind("switch"), this);
Finder->addMatcher(conditionalOperator().bind("condOp"), this);
+
Finder->addMatcher(ifStmt(hasDescendant(ifStmt())).bind("ifWithDescendantIf"),
+ this);
+}
+
+/// Determines whether two statement trees are identical regarding
+/// operators and symbols.
+///
+/// Exceptions: expressions containing macros or functions with possible side
+/// effects are never considered identical.
+/// Limitations: (t + u) and (u + t) are not considered identical.
+/// t*(u + t) and t*u + t*t are not considered identical.
+///
+static bool isIdenticalStmt(const ASTContext &Ctx, const Stmt *Stmt1,
+const Stmt *Stmt2, bool IgnoreSideEffects) {
+
+ if (!Stmt1 || !Stmt2) {
+return !Stmt1 && !Stmt2;
+ }
+
+ // If Stmt1 & Stmt2 are of different class then they are not
+ // identical statements.
+ if (Stmt1->getStmtClass() != Stmt2->getStmtClass())
+return false;
+
+ const Expr *Expr1 = dyn_cast(Stmt1);
+ const Expr *Expr2 = dyn_cast(Stmt2);
+
+ if (Expr1 && Expr2) {
+// If Stmt1 has side effects then don't warn even if expressions
+// are identical.
+if (!IgnoreSideEffects && Expr1->HasSideEffects(Ctx))
+ return false;
+// If either expression comes from a macro then don't warn even if
+// the expressions are identical.
+if ((Expr1->getExprLoc().isMacroID()) || (Expr2->getExprLoc().isMacroID()))
+ return false;
+
+// If all children of two expressions are identical, return true.
+Expr::const_child_iterator I1 = Expr1->child_begin();
+Expr::const_child_iterator I2 = Expr2->child_begin();
+while (I1 != Expr1->child_end() && I2 != Expr2->child_end()) {
+ if (!*I1 || !*I2 || !isIdenticalStmt(Ctx, *I1, *I2, IgnoreSideEffects))
+return false;
+ ++I1;
+ ++I2;
+}
+// If there are different number of children in the statements, return
+// false.
+if (I1 != Expr1->child_end())
+ return false;
+if (I2 != Expr2->child_end())
+ return false;
+ }
+
+ switch (Stmt1->getStmtClass()) {
+ default:
+return false;
+ case Stmt::CallExprClass:
+ case Stmt::ArraySubscriptExprClass:
+ case Stmt::ArraySectionExprClass:
+ case Stmt::OMPArrayShapingExprClass:
+ case Stmt::OMPIteratorExprClass:
+ case Stmt::ImplicitCastExprClass:
+ case Stmt::ParenExprClass:
+ case Stmt::BreakStmtClass:
+ case Stmt::ContinueStmtClass:
+ case Stmt::NullStmtClass:
+return true;
+ case Stmt::CStyleCastExprClass: {
+const CStyleCastExpr *CastExpr1 = cast(Stmt1);
+const CStyleCastExpr *CastExpr2 = cast(Stmt2);
+
+return CastExpr1->getTypeAsWritten() == CastExpr2->getTypeAsWritten();
+ }
+ case Stmt::ReturnStmtClass: {
+const ReturnStmt *ReturnStmt1 = cast(Stmt1);
+const ReturnStmt *ReturnStmt2 = cast(Stmt2);
+
+return isIdenticalStmt(Ctx, ReturnStmt1->getRetValue(),
+ ReturnStmt2->getRetValue(), IgnoreSideEffects);
+ }
+ case Stmt::ForStmtClass: {
+const ForStmt *ForStmt1 = cast(Stm
[clang] [clang-tools-extra] [analyzer] Remove alpha.core.IdenticalExpr Checker (PR #114715)
steakhal wrote: I'm pretty sure last time I've compared this checker to the tidy implementation, I saw slight differences. Have you thoroughly compared the two? Have you seen discrepancies? Speaking of the tests, I'd also prefer migrating existing ones to uncover differences. Unfortunately, frequently our test coverage is not great and because of that I don't think we can drop this unless we/you do a through comparison of the two. BTW I'm all in favor of reducing code duplication, and clang-tidy is a much better place for this rule. https://github.com/llvm/llvm-project/pull/114715 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [analyzer] Remove alpha.core.IdenticalExpr Checker (PR #114715)
vabridgers wrote: @NagyDonat , sounds good. Thank you! https://github.com/llvm/llvm-project/pull/114715 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [analyzer] Remove alpha.core.IdenticalExpr Checker (PR #114715)
NagyDonat wrote: > I did take the cases one by one from identical-expressions.cpp to the tidy > checks and noticed one pattern. It looks to me like identical expressions > utilizing floats were excluded in the tidy checks but found in the static > analysis check. Do you want to retain that through improvements in > misc-redundant-expression, or leave that marked as FIXME? I'm happy to take a > look at an improvement like that. If it's not too difficult, then please improve the tidy checks to cover everything that was covered by the static analyzer checker that we want to remove. (Obviously this doesn't have a major practical importance as I assume that practically nobody used this alpha checker, but it's still nice to say that we preserve every feature.) _(Disclaimer: if you think that the analyzer-only features happen to be buggy and/or completely useless, then don't port them.)_ If the changes are complex and you want to create several separate commits, then I think the natural commit order would be (1) adding the new features in tidy and then (2) removing the analyzer checker in a follow-up commit. However, I'd guess that it's reasonable to do these all in a single commit. https://github.com/llvm/llvm-project/pull/114715 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [analyzer] Remove alpha.core.IdenticalExpr Checker (PR #114715)
@@ -814,6 +814,12 @@ Improvements Moved checkers ^^ +- The checker ``alpha.core.IdenticalExpr`` was removed because it was + duplicated in the clang-tidy checkers ``misc-redundant-expression`` and EugeneZelenko wrote: ```suggestion duplicated in the Clang-Tidy checks ``misc-redundant-expression`` and ``` https://github.com/llvm/llvm-project/pull/114715 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [analyzer] Remove alpha.core.IdenticalExpr Checker (PR #114715)
vabridgers wrote: Hi @NagyDonat, I can port to new and smaller tests - no problem. This was an initial, speculative PR to get the conversation going, especially for comments like this. I'll update the PR. I did take the cases one by one from identical-expressions.cpp to the tidy checks and noticed one pattern. It looks to me like identical expressions utilizing floats were excluded in the tidy checks but found in the static analysis check. Do you want to retain that through improvements in misc-redundant-expression, or leave that marked as FIXME? I'm happy to take a look at an improvement like that. Thanks! - Vince https://github.com/llvm/llvm-project/pull/114715 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [analyzer] Remove alpha.core.IdenticalExpr Checker (PR #114715)
carlosgalvezp wrote: > (@ tidy developers: what would you prefer?) I agree! https://github.com/llvm/llvm-project/pull/114715 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [analyzer] Remove alpha.core.IdenticalExpr Checker (PR #114715)
https://github.com/NagyDonat commented: Thanks for implementing this cleanup commit! I like that you ported the tests from the old checker to the tidy checks. However, as these test files are very large, perhaps it would be better to put these moved tests into stand-alone files instead of adding them at the end of existing files. (@ tidy developers: what would you prefer?) Do I understand it correctly that the two tidy checkers report all the issues that were reported by alpha.core.IdenticalExpr? (Unfortunately github cannot show a clear diff between the old and new version of the moved code.) https://github.com/llvm/llvm-project/pull/114715 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [analyzer] Remove alpha.core.IdenticalExpr Checker (PR #114715)
llvmbot wrote:
@llvm/pr-subscribers-clang
@llvm/pr-subscribers-clang-static-analyzer-1
Author: None (vabridgers)
Changes
This change removes the alpha.core.IdenticalExpr static analysis checker since
it's checks are present in the clang-tidy checks misc-redundant-expression and
bugprone-branch-clone. This check was implemented as a static analysis check
using AST matching, and since alpha and duplicated in 2 clang-tidy checks may
be removed.
---
Patch is 108.47 KiB, truncated to 20.00 KiB below, full version:
https://github.com/llvm/llvm-project/pull/114715.diff
8 Files Affected:
- (modified)
clang-tools-extra/test/clang-tidy/checkers/bugprone/branch-clone.cpp (+547)
- (modified)
clang-tools-extra/test/clang-tidy/checkers/misc/redundant-expression.cpp
(+1325)
- (modified) clang/docs/ReleaseNotes.rst (+6)
- (modified) clang/docs/analyzer/checkers.rst (-30)
- (modified) clang/include/clang/StaticAnalyzer/Checkers/Checkers.td (-4)
- (modified) clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt (-1)
- (removed) clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp (-520)
- (removed) clang/test/Analysis/identical-expressions.cpp (-1564)
``diff
diff --git
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/branch-clone.cpp
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/branch-clone.cpp
index 42231746149f2c..c6af207d795c5d 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/branch-clone.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/branch-clone.cpp
@@ -1069,3 +1069,550 @@ namespace PR62693 {
}
}
}
+
+// Start of identical expressions port
+int func(void)
+{
+ return 0;
+}
+
+int func2(void)
+{
+ return 0;
+}
+
+int funcParam(int a)
+{
+ return 0;
+}
+unsigned test_unsigned(unsigned a) {
+ unsigned b = 1;
+ a = a > 5 ? b : b;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: conditional operator with
identical true and false expressions [bugprone-branch-clone]
+ return a;
+}
+void test_signed() {
+ int a = 0;
+ a = a > 5 ? a : a;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: conditional operator with
identical true and false expressions [bugprone-branch-clone]
+}
+
+void test_bool(bool a) {
+ a = a > 0 ? a : a;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: conditional operator with
identical true and false expressions [bugprone-branch-clone]
+}
+
+void test_float() {
+ float a = 0;
+ float b = 0;
+ a = a > 5 ? a : a;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: conditional operator with
identical true and false expressions [bugprone-branch-clone]
+}
+
+const char *test_string() {
+ float a = 0;
+ return a > 5 ? "abc" : "abc";
+ // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: conditional operator with
identical true and false expressions [bugprone-branch-clone]
+}
+
+void test_unsigned_expr() {
+ unsigned a = 0;
+ unsigned b = 0;
+ a = a > 5 ? a+b : a+b;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: conditional operator with
identical true and false expressions [bugprone-branch-clone]
+}
+
+void test_signed_expr() {
+ int a = 0;
+ int b = 1;
+ a = a > 5 ? a+b : a+b;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: conditional operator with
identical true and false expressions [bugprone-branch-clone]
+}
+
+void test_bool_expr(bool a) {
+ bool b = 0;
+ a = a > 0 ? a&&b : a&&b;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: conditional operator with
identical true and false expressions [bugprone-branch-clone]
+}
+
+void test_unsigned_expr_negative() {
+ unsigned a = 0;
+ unsigned b = 0;
+ a = a > 5 ? a+b : b+a; // no warning
+}
+
+void test_signed_expr_negative() {
+ int a = 0;
+ int b = 1;
+ a = a > 5 ? b+a : a+b; // no warning
+}
+
+void test_bool_expr_negative(bool a) {
+ bool b = 0;
+ a = a > 0 ? a&&b : b&&a; // no warning
+}
+
+void test_float_expr_positive() {
+ float a = 0;
+ float b = 0;
+ a = a > 5 ? a+b : a+b;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: conditional operator with
identical true and false expressions [bugprone-branch-clone]
+}
+
+void test_expr_positive_func() {
+ unsigned a = 0;
+ unsigned b = 1;
+ a = a > 5 ? a+func() : a+func();
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: conditional operator with
identical true and false expressions [bugprone-branch-clone]
+}
+
+void test_expr_negative_func() {
+ unsigned a = 0;
+ unsigned b = 1;
+ a = a > 5 ? a+func() : a+func2(); // no warning
+}
+
+void test_expr_positive_funcParam() {
+ unsigned a = 0;
+ unsigned b = 1;
+ a = a > 5 ? a+funcParam(b) : a+funcParam(b);
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: conditional operator with
identical true and false expressions [bugprone-branch-clone]
+}
+
+void test_expr_negative_funcParam() {
+ unsigned a = 0;
+ unsigned b = 1;
+ a = a > 5 ? a+funcParam(a) : a+funcParam(b); // no warning
+}
+
+void test_expr_positive_inc() {
+ unsigned a = 0;
+ unsigned b = 1;
+ a = a > 5 ? a++ : a++;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warnin
[clang] [clang-tools-extra] [analyzer] Remove alpha.core.IdenticalExpr Checker (PR #114715)
llvmbot wrote:
@llvm/pr-subscribers-clang-tools-extra
Author: None (vabridgers)
Changes
This change removes the alpha.core.IdenticalExpr static analysis checker since
it's checks are present in the clang-tidy checks misc-redundant-expression and
bugprone-branch-clone. This check was implemented as a static analysis check
using AST matching, and since alpha and duplicated in 2 clang-tidy checks may
be removed.
---
Patch is 108.47 KiB, truncated to 20.00 KiB below, full version:
https://github.com/llvm/llvm-project/pull/114715.diff
8 Files Affected:
- (modified)
clang-tools-extra/test/clang-tidy/checkers/bugprone/branch-clone.cpp (+547)
- (modified)
clang-tools-extra/test/clang-tidy/checkers/misc/redundant-expression.cpp
(+1325)
- (modified) clang/docs/ReleaseNotes.rst (+6)
- (modified) clang/docs/analyzer/checkers.rst (-30)
- (modified) clang/include/clang/StaticAnalyzer/Checkers/Checkers.td (-4)
- (modified) clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt (-1)
- (removed) clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp (-520)
- (removed) clang/test/Analysis/identical-expressions.cpp (-1564)
``diff
diff --git
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/branch-clone.cpp
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/branch-clone.cpp
index 42231746149f2c..c6af207d795c5d 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/branch-clone.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/branch-clone.cpp
@@ -1069,3 +1069,550 @@ namespace PR62693 {
}
}
}
+
+// Start of identical expressions port
+int func(void)
+{
+ return 0;
+}
+
+int func2(void)
+{
+ return 0;
+}
+
+int funcParam(int a)
+{
+ return 0;
+}
+unsigned test_unsigned(unsigned a) {
+ unsigned b = 1;
+ a = a > 5 ? b : b;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: conditional operator with
identical true and false expressions [bugprone-branch-clone]
+ return a;
+}
+void test_signed() {
+ int a = 0;
+ a = a > 5 ? a : a;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: conditional operator with
identical true and false expressions [bugprone-branch-clone]
+}
+
+void test_bool(bool a) {
+ a = a > 0 ? a : a;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: conditional operator with
identical true and false expressions [bugprone-branch-clone]
+}
+
+void test_float() {
+ float a = 0;
+ float b = 0;
+ a = a > 5 ? a : a;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: conditional operator with
identical true and false expressions [bugprone-branch-clone]
+}
+
+const char *test_string() {
+ float a = 0;
+ return a > 5 ? "abc" : "abc";
+ // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: conditional operator with
identical true and false expressions [bugprone-branch-clone]
+}
+
+void test_unsigned_expr() {
+ unsigned a = 0;
+ unsigned b = 0;
+ a = a > 5 ? a+b : a+b;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: conditional operator with
identical true and false expressions [bugprone-branch-clone]
+}
+
+void test_signed_expr() {
+ int a = 0;
+ int b = 1;
+ a = a > 5 ? a+b : a+b;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: conditional operator with
identical true and false expressions [bugprone-branch-clone]
+}
+
+void test_bool_expr(bool a) {
+ bool b = 0;
+ a = a > 0 ? a&&b : a&&b;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: conditional operator with
identical true and false expressions [bugprone-branch-clone]
+}
+
+void test_unsigned_expr_negative() {
+ unsigned a = 0;
+ unsigned b = 0;
+ a = a > 5 ? a+b : b+a; // no warning
+}
+
+void test_signed_expr_negative() {
+ int a = 0;
+ int b = 1;
+ a = a > 5 ? b+a : a+b; // no warning
+}
+
+void test_bool_expr_negative(bool a) {
+ bool b = 0;
+ a = a > 0 ? a&&b : b&&a; // no warning
+}
+
+void test_float_expr_positive() {
+ float a = 0;
+ float b = 0;
+ a = a > 5 ? a+b : a+b;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: conditional operator with
identical true and false expressions [bugprone-branch-clone]
+}
+
+void test_expr_positive_func() {
+ unsigned a = 0;
+ unsigned b = 1;
+ a = a > 5 ? a+func() : a+func();
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: conditional operator with
identical true and false expressions [bugprone-branch-clone]
+}
+
+void test_expr_negative_func() {
+ unsigned a = 0;
+ unsigned b = 1;
+ a = a > 5 ? a+func() : a+func2(); // no warning
+}
+
+void test_expr_positive_funcParam() {
+ unsigned a = 0;
+ unsigned b = 1;
+ a = a > 5 ? a+funcParam(b) : a+funcParam(b);
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: conditional operator with
identical true and false expressions [bugprone-branch-clone]
+}
+
+void test_expr_negative_funcParam() {
+ unsigned a = 0;
+ unsigned b = 1;
+ a = a > 5 ? a+funcParam(a) : a+funcParam(b); // no warning
+}
+
+void test_expr_positive_inc() {
+ unsigned a = 0;
+ unsigned b = 1;
+ a = a > 5 ? a++ : a++;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: conditional operator with
ide
