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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
