[clang-tools-extra] [clang-tidy] ExprSequence: Handle ternary operators. (PR #132913)
via cfe-commits
cfe-commits at lists.llvm.org
Tue Mar 25 03:44:15 PDT 2025
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang-tidy
Author: Clement Courbet (legrosbuffle)
<details>
<summary>Changes</summary>
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.
---
Full diff: https://github.com/llvm/llvm-project/pull/132913.diff
4 Files Affected:
- (modified) clang-tools-extra/clang-tidy/utils/ExprSequence.cpp (+29-19)
- (modified) clang-tools-extra/clang-tidy/utils/ExprSequence.h (+4-5)
- (modified) clang-tools-extra/unittests/clang-tidy/CMakeLists.txt (+2)
- (added) clang-tools-extra/unittests/clang-tidy/ExprSequenceTest.cpp (+111)
``````````diff
diff --git a/clang-tools-extra/clang-tidy/utils/ExprSequence.cpp b/clang-tools-extra/clang-tidy/utils/ExprSequence.cpp
index 145a5fe378b3e..cef70171999b5 100644
--- a/clang-tools-extra/clang-tidy/utils/ExprSequence.cpp
+++ b/clang-tools-extra/clang-tidy/utils/ExprSequence.cpp
@@ -95,10 +95,16 @@ 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 +172,7 @@ 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 +182,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 +199,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 +209,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 +220,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 +230,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 +239,34 @@ 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..67d2d7e149d9f
--- /dev/null
+++ b/clang-tools-extra/unittests/clang-tidy/ExprSequenceTest.cpp
@@ -0,0 +1,111 @@
+//===---- 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
``````````
</details>
https://github.com/llvm/llvm-project/pull/132913
More information about the cfe-commits
mailing list