[PATCH] D42624: [clang-tidy] Add a utility Matcher to match the next statement within a statement sequence
aaron.ballman added a comment. In https://reviews.llvm.org/D42624#1031073, @sylvestre.ledru wrote: > @aaron.ballman Hello, Do you think it is ready to land? Thanks @alexfh was asking to make this matcher local to the check rather than add it to Matchers.h, so I think this patch should be abandoned and rolled into https://reviews.llvm.org/D37014 directly, but the dependency is fine to pick up in that patch. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D42624 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42624: [clang-tidy] Add a utility Matcher to match the next statement within a statement sequence
sylvestre.ledru added a comment. @aaron.ballman Hello, Do you think it is ready to land? Thanks Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D42624 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42624: [clang-tidy] Add a utility Matcher to match the next statement within a statement sequence
alexfh added a comment. In https://reviews.llvm.org/D42624#1027506, @tbourvon wrote: > @alexfh What is your opinion regarding adding this dependency? I'm fine with the dependency, and I'd probably make the matcher local to the check until there is at least one more use case for it in the codebase. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D42624 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42624: [clang-tidy] Add a utility Matcher to match the next statement within a statement sequence
tbourvon added a comment. @alexfh What is your opinion regarding adding this dependency? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D42624 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42624: [clang-tidy] Add a utility Matcher to match the next statement within a statement sequence
aaron.ballman added inline comments. Comment at: clang-tidy/utils/Matchers.h:16 +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/Analysis/CFG.h" tbourvon wrote: > aaron.ballman wrote: > > This will require linking in the clangAnalysis library as well; are we sure > > we want to take on this dependency here for all matchers? > Do you have a specific solution in mind? We could make the matcher local to > the check it is being used in (see D37014), but I think it could prove useful > for other checks... No solution in mind -- I was mostly wondering if @alexfh was okay picking up this dependency or not, because it will impact all the clang-tidy modules. Comment at: clang-tidy/utils/Matchers.h:58-60 + // We get the first parent, making sure that we're not in a case statement + // not in a compound statement directly inside a switch, because this causes + // the buildCFG call to crash. tbourvon wrote: > aaron.ballman wrote: > > Why does it cause the crash? Should that crash not be fixed instead of > > applying this workaround? > I'm not entirely sure if this is expected behavior or not. In terms of AST, > `switch` statements are a bit special in terms of how they are represented > (each case contains all the subsequent cases as its children IIRC). > There probably is a way to make the CFG work in these cases, but I honestly > don't have the time to look into that and attempt a fix. Couldn't this be > good enough for now, maybe with a FIXME? I'm uncomfortable simply working around known crashes rather than fixing them, even with FIXME comments, because that just means we accrue more technical debt with no plan in place to pay it down in the future. However, I'll let @alexfh make the final call on it. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D42624 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42624: [clang-tidy] Add a utility Matcher to match the next statement within a statement sequence
tbourvon added inline comments. Comment at: clang-tidy/utils/Matchers.h:16 +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/Analysis/CFG.h" aaron.ballman wrote: > This will require linking in the clangAnalysis library as well; are we sure > we want to take on this dependency here for all matchers? Do you have a specific solution in mind? We could make the matcher local to the check it is being used in (see D37014), but I think it could prove useful for other checks... Comment at: clang-tidy/utils/Matchers.h:58-60 + // We get the first parent, making sure that we're not in a case statement + // not in a compound statement directly inside a switch, because this causes + // the buildCFG call to crash. aaron.ballman wrote: > Why does it cause the crash? Should that crash not be fixed instead of > applying this workaround? I'm not entirely sure if this is expected behavior or not. In terms of AST, `switch` statements are a bit special in terms of how they are represented (each case contains all the subsequent cases as its children IIRC). There probably is a way to make the CFG work in these cases, but I honestly don't have the time to look into that and attempt a fix. Couldn't this be good enough for now, maybe with a FIXME? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D42624 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42624: [clang-tidy] Add a utility Matcher to match the next statement within a statement sequence
aaron.ballman added inline comments.
Comment at: clang-tidy/utils/Matchers.h:16
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Analysis/CFG.h"
This will require linking in the clangAnalysis library as well; are we sure we
want to take on this dependency here for all matchers?
Comment at: clang-tidy/utils/Matchers.h:58-60
+ // We get the first parent, making sure that we're not in a case statement
+ // not in a compound statement directly inside a switch, because this causes
+ // the buildCFG call to crash.
Why does it cause the crash? Should that crash not be fixed instead of applying
this workaround?
Comment at: clang-tidy/utils/Matchers.h:70
+
+ // We build a Control Flow Graph (CFG) from the parent statement.
+ std::unique_ptr StatementCFG =
No need for the parenthetical. It's generally understood what a CFG is, so you
can just use the acronym.
Comment at: clang-tidy/utils/Matchers.h:74-75
+ CFG::BuildOptions());
+
+ if (!StatementCFG) {
+return false;
Elide braces (here and elsewhere).
Comment at: clang-tidy/utils/Matchers.h:81
+ // all the possible branches of the code and therefore cover all statements.
+ for (auto& Block : *StatementCFG) {
+if (!Block) {
Formatting (here and elsewhere): you should run the patch through clang-format.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42624
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42624: [clang-tidy] Add a utility Matcher to match the next statement within a statement sequence
tbourvon created this revision.
tbourvon added a reviewer: lebedev.ri.
tbourvon added a project: clang-tools-extra.
Herald added subscribers: hintonda, xazax.hun, mgorny.
This adds a utility matcher (which is placed in `util/Matchers.h`, because it
uses functions from `ASTMatchFinder.h` which cannot be used from
`ASTMatchers.h`) used by https://reviews.llvm.org/D37014).
https://reviews.llvm.org/D42624
Files:
clang-tidy/utils/Matchers.h
unittests/clang-tidy/CMakeLists.txt
unittests/clang-tidy/MatchersUtilsTest.cpp
Index: unittests/clang-tidy/MatchersUtilsTest.cpp
===
--- /dev/null
+++ unittests/clang-tidy/MatchersUtilsTest.cpp
@@ -0,0 +1,25 @@
+#include "../../../unittests/ASTMatchers/ASTMatchersTest.h"
+#include "utils/Matchers.h"
+
+namespace clang {
+namespace tidy {
+namespace test {
+
+using namespace ast_matchers;
+using namespace matchers;
+
+TEST(StatementMatcher, HasSuccessor) {
+ StatementMatcher DeclHasSuccessorReturnStmt =
+declStmt(hasSuccessor(returnStmt()));
+
+ EXPECT_TRUE(matches(
+"void foo() { int bar; return; }",
+DeclHasSuccessorReturnStmt));
+ EXPECT_TRUE(notMatches(
+"void foo() { int bar; bar++; return; }",
+DeclHasSuccessorReturnStmt));
+}
+
+}
+}
+}
Index: unittests/clang-tidy/CMakeLists.txt
===
--- unittests/clang-tidy/CMakeLists.txt
+++ unittests/clang-tidy/CMakeLists.txt
@@ -12,6 +12,7 @@
IncludeInserterTest.cpp
GoogleModuleTest.cpp
LLVMModuleTest.cpp
+ MatchersUtilsTest.cpp
NamespaceAliaserTest.cpp
ObjCModuleTest.cpp
OverlappingReplacementsTest.cpp
Index: clang-tidy/utils/Matchers.h
===
--- clang-tidy/utils/Matchers.h
+++ clang-tidy/utils/Matchers.h
@@ -12,6 +12,8 @@
#include "TypeTraits.h"
#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Analysis/CFG.h"
namespace clang {
namespace tidy {
@@ -48,6 +50,67 @@
return referenceType(pointee(qualType(isConstQualified(;
}
+// Matches the next statement within the parent statement sequence.
+AST_MATCHER_P(Stmt, hasSuccessor,
+ ast_matchers::internal::Matcher, InnerMatcher) {
+ using namespace ast_matchers;
+
+ // We get the first parent, making sure that we're not in a case statement
+ // not in a compound statement directly inside a switch, because this causes
+ // the buildCFG call to crash.
+ auto Parent = selectFirst(
+"parent",
+match(
+ stmt(hasAncestor(stmt(
+unless(caseStmt()),
+unless(compoundStmt(hasParent(switchStmt(,
+stmt().bind("parent",
+Node, Finder->getASTContext()));
+
+ // We build a Control Flow Graph (CFG) from the parent statement.
+ std::unique_ptr StatementCFG =
+CFG::buildCFG(nullptr, const_cast(Parent), >getASTContext(),
+ CFG::BuildOptions());
+
+ if (!StatementCFG) {
+return false;
+ }
+
+ // We iterate through all the CFGBlocks, which basically means that we go over
+ // all the possible branches of the code and therefore cover all statements.
+ for (auto& Block : *StatementCFG) {
+if (!Block) {
+ continue;
+}
+
+// We iterate through all the statements of the block.
+bool ReturnNextStmt = false;
+for (auto& BlockItem : *Block) {
+ Optional CFGStatement = BlockItem.getAs();
+ if (!CFGStatement) {
+if (ReturnNextStmt) {
+ return false;
+}
+
+continue;
+ }
+
+ // If we found the next statement, we apply the inner matcher and return
+ // the result.
+ if (ReturnNextStmt) {
+return InnerMatcher.matches(*CFGStatement->getStmt(), Finder, Builder);
+ }
+
+ if (CFGStatement->getStmt() == ) {
+ReturnNextStmt = true;
+ }
+}
+ }
+
+ // If we didn't find a successor, we just return false.
+ return false;
+}
+
} // namespace matchers
} // namespace tidy
} // namespace clang
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
