https://github.com/legrosbuffle updated
https://github.com/llvm/llvm-project/pull/132913
>From dcb97b74e78148e03f7749f436f01d5488c11ae1 Mon Sep 17 00:00:00 2001
From: Clement Courbet <cour...@google.com>
Date: Tue, 25 Mar 2025 09:42:53 +0000
Subject: [PATCH] [clang-tidy] ExprSequence: Handle ternary operators.
The first operand in the conditional operator `?:` is sequenced before
the second or third operand.
This was probably not implemented because it did not fit in the "single
successor" model of `getSuccessor` (the conditional operator has two
unsequenced successors). Switch to potentially returning several
successors in `getSuccessors` and implement the conditional operator.
Add unit tests.
---
.../clang-tidy/utils/ExprSequence.cpp | 51 +++++---
.../clang-tidy/utils/ExprSequence.h | 9 +-
.../unittests/clang-tidy/CMakeLists.txt | 2 +
.../unittests/clang-tidy/ExprSequenceTest.cpp | 112 ++++++++++++++++++
4 files changed, 150 insertions(+), 24 deletions(-)
create mode 100644 clang-tools-extra/unittests/clang-tidy/ExprSequenceTest.cpp
diff --git a/clang-tools-extra/clang-tidy/utils/ExprSequence.cpp
b/clang-tools-extra/clang-tidy/utils/ExprSequence.cpp
index 145a5fe378b3e..07a02702f8571 100644
--- a/clang-tools-extra/clang-tidy/utils/ExprSequence.cpp
+++ b/clang-tools-extra/clang-tidy/utils/ExprSequence.cpp
@@ -95,10 +95,17 @@ bool ExprSequence::inSequence(const Stmt *Before, const
Stmt *After) const {
// If 'After' is in the subtree of the siblings that follow 'Before' in the
// chain of successors, we know that 'After' is sequenced after 'Before'.
- for (const Stmt *Successor = getSequenceSuccessor(Before); Successor;
- Successor = getSequenceSuccessor(Successor)) {
- if (isDescendantOrEqual(After, Successor, Context))
- return true;
+ {
+ SmallVector<const Stmt *, 2> Stack = {Before};
+ while (!Stack.empty()) {
+ const Stmt *Node = Stack.back();
+ Stack.pop_back();
+ for (const Stmt *Successor : getSequenceSuccessors(Node)) {
+ if (isDescendantOrEqual(After, Successor, Context))
+ return true;
+ Stack.push_back(Successor);
+ }
+ }
}
SmallVector<const Stmt *, 1> BeforeParents = getParentStmts(Before, Context);
@@ -166,7 +173,8 @@ bool ExprSequence::potentiallyAfter(const Stmt *After,
return !inSequence(After, Before);
}
-const Stmt *ExprSequence::getSequenceSuccessor(const Stmt *S) const {
+llvm::SmallVector<const Stmt *, 2>
+ExprSequence::getSequenceSuccessors(const Stmt *S) const {
for (const Stmt *Parent : getParentStmts(S, Context)) {
// If a statement has multiple parents, make sure we're using the parent
// that lies within the sub-tree under Root.
@@ -176,14 +184,14 @@ const Stmt *ExprSequence::getSequenceSuccessor(const Stmt
*S) const {
if (const auto *BO = dyn_cast<BinaryOperator>(Parent)) {
// Comma operator: Right-hand side is sequenced after the left-hand side.
if (BO->getLHS() == S && BO->getOpcode() == BO_Comma)
- return BO->getRHS();
+ return {BO->getRHS()};
} else if (const auto *InitList = dyn_cast<InitListExpr>(Parent)) {
// Initializer list: Each initializer clause is sequenced after the
// clauses that precede it.
for (const InitListExpr *Form : getAllInitListForms(InitList)) {
for (unsigned I = 1; I < Form->getNumInits(); ++I) {
if (Form->getInit(I - 1) == S) {
- return Form->getInit(I);
+ return {Form->getInit(I)};
}
}
}
@@ -193,7 +201,7 @@ const Stmt *ExprSequence::getSequenceSuccessor(const Stmt
*S) const {
if (ConstructExpr->isListInitialization()) {
for (unsigned I = 1; I < ConstructExpr->getNumArgs(); ++I) {
if (ConstructExpr->getArg(I - 1) == S) {
- return ConstructExpr->getArg(I);
+ return {ConstructExpr->getArg(I)};
}
}
}
@@ -203,7 +211,7 @@ const Stmt *ExprSequence::getSequenceSuccessor(const Stmt
*S) const {
const Stmt *Previous = nullptr;
for (const auto *Child : Compound->body()) {
if (Previous == S)
- return Child;
+ return {Child};
Previous = Child;
}
} else if (const auto *TheDeclStmt = dyn_cast<DeclStmt>(Parent)) {
@@ -214,7 +222,7 @@ const Stmt *ExprSequence::getSequenceSuccessor(const Stmt
*S) const {
if (const auto *TheVarDecl = dyn_cast<VarDecl>(TheDecl)) {
if (const Expr *Init = TheVarDecl->getInit()) {
if (PreviousInit == S)
- return Init;
+ return {Init};
PreviousInit = Init;
}
}
@@ -224,7 +232,7 @@ const Stmt *ExprSequence::getSequenceSuccessor(const Stmt
*S) const {
// body. (We need this rule because these get placed in the same
// CFGBlock.)
if (S == ForRange->getLoopVarStmt())
- return ForRange->getBody();
+ return {ForRange->getBody()};
} else if (const auto *TheIfStmt = dyn_cast<IfStmt>(Parent)) {
// If statement:
// - Sequence init statement before variable declaration, if present;
@@ -233,30 +241,35 @@ const Stmt *ExprSequence::getSequenceSuccessor(const Stmt
*S) const {
// initialize it) before the evaluation of the condition.
if (S == TheIfStmt->getInit()) {
if (TheIfStmt->getConditionVariableDeclStmt() != nullptr)
- return TheIfStmt->getConditionVariableDeclStmt();
- return TheIfStmt->getCond();
+ return {TheIfStmt->getConditionVariableDeclStmt()};
+ return {TheIfStmt->getCond()};
}
if (S == TheIfStmt->getConditionVariableDeclStmt())
- return TheIfStmt->getCond();
+ return {TheIfStmt->getCond()};
} else if (const auto *TheSwitchStmt = dyn_cast<SwitchStmt>(Parent)) {
// Ditto for switch statements.
if (S == TheSwitchStmt->getInit()) {
if (TheSwitchStmt->getConditionVariableDeclStmt() != nullptr)
- return TheSwitchStmt->getConditionVariableDeclStmt();
- return TheSwitchStmt->getCond();
+ return {TheSwitchStmt->getConditionVariableDeclStmt()};
+ return {TheSwitchStmt->getCond()};
}
if (S == TheSwitchStmt->getConditionVariableDeclStmt())
- return TheSwitchStmt->getCond();
+ return {TheSwitchStmt->getCond()};
} else if (const auto *TheWhileStmt = dyn_cast<WhileStmt>(Parent)) {
// While statement: Sequence variable declaration (along with the
// expression used to initialize it) before the evaluation of the
// condition.
if (S == TheWhileStmt->getConditionVariableDeclStmt())
- return TheWhileStmt->getCond();
+ return {TheWhileStmt->getCond()};
+ } else if (const auto *TheCondStmt =
+ dyn_cast<ConditionalOperator>(Parent)) {
+ // Conditional operator: condition is sequenced before both arms.
+ if (S == TheCondStmt->getCond())
+ return {TheCondStmt->getTrueExpr(), TheCondStmt->getFalseExpr()};
}
}
- return nullptr;
+ return {};
}
const Stmt *ExprSequence::resolveSyntheticStmt(const Stmt *S) const {
diff --git a/clang-tools-extra/clang-tidy/utils/ExprSequence.h
b/clang-tools-extra/clang-tidy/utils/ExprSequence.h
index 6531e1876c4fe..5cbd4c3564f20 100644
--- a/clang-tools-extra/clang-tidy/utils/ExprSequence.h
+++ b/clang-tools-extra/clang-tidy/utils/ExprSequence.h
@@ -78,15 +78,14 @@ class ExprSequence {
bool potentiallyAfter(const Stmt *After, const Stmt *Before) const;
private:
- // Returns the sibling of \p S (if any) that is directly sequenced after \p
S,
- // or nullptr if no such sibling exists. For example, if \p S is the child of
- // a `CompoundStmt`, this would return the Stmt that directly follows \p S in
- // the `CompoundStmt`.
+ // // Returns the siblings of \p S (if any) that are directly sequenced after
+ // \p S. For example, if \p S is the child of a `CompoundStmt`, this would
+ // return the Stmt that directly follows \p S in the `CompoundStmt`.
//
// As the sequencing of many constructs that change control flow is already
// encoded in the `CFG`, this function only implements the sequencing rules
// for those constructs where sequencing cannot be inferred from the `CFG`.
- const Stmt *getSequenceSuccessor(const Stmt *S) const;
+ llvm::SmallVector<const Stmt *, 2> getSequenceSuccessors(const Stmt *S)
const;
const Stmt *resolveSyntheticStmt(const Stmt *S) const;
diff --git a/clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
b/clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
index 3304924d39757..076517a77d356 100644
--- a/clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
+++ b/clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
@@ -22,6 +22,7 @@ add_extra_unittest(ClangTidyTests
ClangTidyDiagnosticConsumerTest.cpp
ClangTidyOptionsTest.cpp
DeclRefExprUtilsTest.cpp
+ ExprSequenceTest.cpp
IncludeCleanerTest.cpp
IncludeInserterTest.cpp
GlobListTest.cpp
@@ -39,6 +40,7 @@ add_extra_unittest(ClangTidyTests
clang_target_link_libraries(ClangTidyTests
PRIVATE
+ clangAnalysis
clangAST
clangASTMatchers
clangBasic
diff --git a/clang-tools-extra/unittests/clang-tidy/ExprSequenceTest.cpp
b/clang-tools-extra/unittests/clang-tidy/ExprSequenceTest.cpp
new file mode 100644
index 0000000000000..3df223e1aec33
--- /dev/null
+++ b/clang-tools-extra/unittests/clang-tidy/ExprSequenceTest.cpp
@@ -0,0 +1,112 @@
+//===---- UsingInserterTest.cpp - clang-tidy ----------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "../clang-tidy/utils/ExprSequence.h"
+
+#include "ClangTidyTest.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace tidy {
+namespace utils {
+
+// Checks that expression `before` is sequenced before `after`.
+// Check sthat expression `unseq1` is not sequenced before or sequenced
+// after `unseq2`.
+class ExprSequenceCheck : public clang::tidy::ClangTidyCheck {
+public:
+ ExprSequenceCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context) {}
+ void registerMatchers(clang::ast_matchers::MatchFinder *Finder) override {
+ using namespace clang::ast_matchers;
+ const auto RefTo = [](StringRef name) {
+ return declRefExpr(to(varDecl(hasName(name)))).bind(name);
+ };
+ Finder->addMatcher(functionDecl(hasDescendant(RefTo("before")),
+ hasDescendant(RefTo("after")))
+ .bind("fn"),
+ this);
+ Finder->addMatcher(functionDecl(hasDescendant(RefTo("unseq1")),
+ hasDescendant(RefTo("unseq2")))
+ .bind("fn"),
+ this);
+ }
+ void
+ check(const clang::ast_matchers::MatchFinder::MatchResult &Result) override {
+ const auto *Fn = Result.Nodes.getNodeAs<clang::FunctionDecl>("fn");
+ const auto *Before = Result.Nodes.getNodeAs<clang::Expr>("before");
+ const auto *After = Result.Nodes.getNodeAs<clang::Expr>("after");
+ const auto *Unseq1 = Result.Nodes.getNodeAs<clang::Expr>("unseq1");
+ const auto *Unseq2 = Result.Nodes.getNodeAs<clang::Expr>("unseq2");
+
+ CFG::BuildOptions Options;
+ Options.AddImplicitDtors = true;
+ Options.AddTemporaryDtors = true;
+ std::unique_ptr<CFG> TheCFG =
+ CFG::buildCFG(nullptr, Fn->getBody(), Result.Context, Options);
+ ASSERT_TRUE(TheCFG != nullptr);
+
+ ExprSequence Seq(TheCFG.get(), Fn->getBody(), Result.Context);
+
+ if (Before && !Seq.inSequence(Before, After)) {
+ diag(Before->getBeginLoc(),
+ "[seq]expected 'before' to be sequenced before 'after'");
+ }
+ if (Unseq1 &&
+ (Seq.inSequence(Unseq1, Unseq2) || Seq.inSequence(Unseq2, Unseq1))) {
+ diag(Before->getBeginLoc(),
+ "[seq]expected 'unseq1' and 'unseq2' to not be sequenced");
+ }
+ }
+};
+
+void runChecker(StringRef Code) {
+ std::vector<ClangTidyError> Errors;
+
+ std::string result = test::runCheckOnCode<ExprSequenceCheck>(
+ Code, &Errors, "foo.cc", {}, ClangTidyOptions());
+
+ bool HasSeqError = false;
+ for (const ClangTidyError &E : Errors) {
+ // Ignore non-seq warnings.
+ StringRef Msg(E.Message.Message);
+ if (!Msg.consume_front("[seq]"))
+ continue;
+ llvm::errs() << Msg << "\nIn code:\n" << Code << "\n";
+ HasSeqError = true;
+ }
+ EXPECT_FALSE(HasSeqError);
+}
+
+TEST(ExprSequenceTest, Unseq) {
+ runChecker("int f(int unseq1, int unseq2) { return unseq1 + unseq2; }");
+}
+
+TEST(ExprSequenceTest, Comma) {
+ runChecker("int f(int before, int after) { return before, after; }");
+}
+
+TEST(ExprSequenceTest, Ctor) {
+ runChecker("struct S { S(int, int, int); };"
+ "S f(int before, int after) { return S{before, 3, after}; }");
+ runChecker("struct S { S(int, int, int); };"
+ "S f(int unseq1, int unseq2) { return S(unseq1, 3, unseq2); }");
+}
+
+TEST(ExprSequenceTest, ConditionalOperator) {
+ runChecker("int f(bool before, int after) { return before ? after : 3; }");
+ runChecker("int f(bool before, int after) { return before ? 3 : after; }");
+ runChecker(
+ "int f(bool c, int unseq1, int unseq2) { return c ? unseq1 : unseq2; }");
+}
+
+} // namespace utils
+} // namespace tidy
+} // namespace clang
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
