Author: Yitzhak Mandelbaum Date: 2020-03-05T14:48:40-05:00 New Revision: c359f9537ffb17c4f40a933980ddb515d7ee923b
URL: https://github.com/llvm/llvm-project/commit/c359f9537ffb17c4f40a933980ddb515d7ee923b DIFF: https://github.com/llvm/llvm-project/commit/c359f9537ffb17c4f40a933980ddb515d7ee923b.diff LOG: [AST Matchers] Restrict `optionally` matcher to a single argument. Summary: Currently, `optionally` can take multiple arguments, which commits it to a particular strategy for those arguments (in this case, "for each"). We limit the matcher to a single argument, which avoids any potential confusion and simplifies the implementation. The user can retrieve multiple-argument optionality, by explicitly using the desired operator (like `forEach`, `anyOf`, `allOf`, etc.) with all children wrapped in `optionally`. Reviewers: sbenza, aaron.ballman Subscribers: cfe-commits Tags: #clang Differential Revision: https://reviews.llvm.org/D75556 Added: Modified: clang/docs/LibASTMatchersReference.html clang/include/clang/ASTMatchers/ASTMatchers.h clang/lib/ASTMatchers/ASTMatchersInternal.cpp clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp Removed: ################################################################################ diff --git a/clang/docs/LibASTMatchersReference.html b/clang/docs/LibASTMatchersReference.html index f8c56d7d9efc..dca7aa5c2cf7 100644 --- a/clang/docs/LibASTMatchersReference.html +++ b/clang/docs/LibASTMatchersReference.html @@ -4793,14 +4793,12 @@ <h2 id="traversal-matchers">AST Traversal Matchers</h2> </pre></td></tr> -<tr><td>Matcher<*></td><td class="name" onclick="toggle('optionally0')"><a name="optionally0Anchor">optionally</a></td><td>Matcher<*>, ..., Matcher<*></td></tr> -<tr><td colspan="4" class="doc" id="optionally0"><pre>Matches any node regardless of the submatchers. +<tr><td>Matcher<*></td><td class="name" onclick="toggle('optionally0')"><a name="optionally0Anchor">optionally</a></td><td>Matcher<*></td></tr> +<tr><td colspan="4" class="doc" id="optionally0"><pre>Matches any node regardless of the submatcher. -However, optionally will generate a result binding for each matching -submatcher. - -Useful when additional information which may or may not present about a -main matching node is desired. +However, optionally will retain any bindings generated by the submatcher. +Useful when additional information which may or may not present about a main +matching node is desired. For example, in: class Foo { diff --git a/clang/include/clang/ASTMatchers/ASTMatchers.h b/clang/include/clang/ASTMatchers/ASTMatchers.h index d45825de67db..da7e23052b28 100644 --- a/clang/include/clang/ASTMatchers/ASTMatchers.h +++ b/clang/include/clang/ASTMatchers/ASTMatchers.h @@ -2586,13 +2586,11 @@ extern const internal::VariadicOperatorMatcherFunc< 2, std::numeric_limits<unsigned>::max()> allOf; -/// Matches any node regardless of the submatchers. +/// Matches any node regardless of the submatcher. /// -/// However, \c optionally will generate a result binding for each matching -/// submatcher. -/// -/// Useful when additional information which may or may not present about a -/// main matching node is desired. +/// However, \c optionally will retain any bindings generated by the submatcher. +/// Useful when additional information which may or may not present about a main +/// matching node is desired. /// /// For example, in: /// \code @@ -2612,9 +2610,7 @@ extern const internal::VariadicOperatorMatcherFunc< /// member named "bar" in that class. /// /// Usable as: Any Matcher -extern const internal::VariadicOperatorMatcherFunc< - 1, std::numeric_limits<unsigned>::max()> - optionally; +extern const internal::VariadicOperatorMatcherFunc<1, 1> optionally; /// Matches sizeof (C99), alignof (C++11) and vec_step (OpenCL) /// diff --git a/clang/lib/ASTMatchers/ASTMatchersInternal.cpp b/clang/lib/ASTMatchers/ASTMatchersInternal.cpp index 6f4132c19aed..033a5f19fd8c 100644 --- a/clang/lib/ASTMatchers/ASTMatchersInternal.cpp +++ b/clang/lib/ASTMatchers/ASTMatchersInternal.cpp @@ -346,18 +346,11 @@ bool OptionallyVariadicOperator(const DynTypedNode &DynNode, ASTMatchFinder *Finder, BoundNodesTreeBuilder *Builder, ArrayRef<DynTypedMatcher> InnerMatchers) { - BoundNodesTreeBuilder Result; - bool Matched = false; - for (const DynTypedMatcher &InnerMatcher : InnerMatchers) { - BoundNodesTreeBuilder BuilderInner(*Builder); - if (InnerMatcher.matches(DynNode, Finder, &BuilderInner)) { - Matched = true; - Result.addMatch(BuilderInner); - } - } - // If there were no matches, we can't assign to `*Builder`; we'd (incorrectly) - // clear it because `Result` is empty. - if (Matched) + if (InnerMatchers.size() != 1) + return false; + + BoundNodesTreeBuilder Result(*Builder); + if (InnerMatchers[0].matches(DynNode, Finder, &Result)) *Builder = std::move(Result); return true; } @@ -859,9 +852,8 @@ const internal::VariadicOperatorMatcherFunc< const internal::VariadicOperatorMatcherFunc< 2, std::numeric_limits<unsigned>::max()> allOf = {internal::DynTypedMatcher::VO_AllOf}; -const internal::VariadicOperatorMatcherFunc< - 1, std::numeric_limits<unsigned>::max()> - optionally = {internal::DynTypedMatcher::VO_Optionally}; +const internal::VariadicOperatorMatcherFunc<1, 1> optionally = { + internal::DynTypedMatcher::VO_Optionally}; const internal::VariadicFunction<internal::Matcher<NamedDecl>, StringRef, internal::hasAnyNameFunc> hasAnyName = {}; diff --git a/clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp b/clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp index edfd1c3fbd91..c3c770a7dffd 100644 --- a/clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp +++ b/clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp @@ -2007,9 +2007,8 @@ TEST(EachOf, BehavesLikeAnyOfUnlessBothMatch) { TEST(Optionally, SubmatchersDoNotMatch) { EXPECT_TRUE(matchAndVerifyResultFalse( "class A { int a; int b; };", - recordDecl(optionally(has(fieldDecl(hasName("c")).bind("v")), - has(fieldDecl(hasName("d")).bind("v")))), - std::make_unique<VerifyIdIsBoundTo<FieldDecl>>("v"))); + recordDecl(optionally(has(fieldDecl(hasName("c")).bind("c")))), + std::make_unique<VerifyIdIsBoundTo<FieldDecl>>("c"))); } // Regression test. @@ -2028,14 +2027,8 @@ TEST(Optionally, SubmatchersDoNotMatchButPreserveBindings) { TEST(Optionally, SubmatchersMatch) { EXPECT_TRUE(matchAndVerifyResultTrue( "class A { int a; int c; };", - recordDecl(optionally(has(fieldDecl(hasName("a")).bind("v")), - has(fieldDecl(hasName("b")).bind("v")))), - std::make_unique<VerifyIdIsBoundTo<FieldDecl>>("v", 1))); - EXPECT_TRUE(matchAndVerifyResultTrue( - "class A { int c; int b; };", - recordDecl(optionally(has(fieldDecl(hasName("c")).bind("v")), - has(fieldDecl(hasName("b")).bind("v")))), - std::make_unique<VerifyIdIsBoundTo<FieldDecl>>("v", 2))); + recordDecl(optionally(has(fieldDecl(hasName("a")).bind("v")))), + std::make_unique<VerifyIdIsBoundTo<FieldDecl>>("v"))); } TEST(IsTemplateInstantiation, MatchesImplicitClassTemplateInstantiation) { _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
