[PATCH] D94880: Add clang-query support for mapAnyOf

[Stephen Kelly via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG04b69d9a6013: Add clang-query support for mapAnyOf (authored 
by stephenkelly).

Changed prior to commit:
  https://reviews.llvm.org/D94880?vs=320774=321989#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94880/new/

https://reviews.llvm.org/D94880

Files:
  clang/include/clang/ASTMatchers/Dynamic/Diagnostics.h
  clang/include/clang/ASTMatchers/Dynamic/Parser.h
  clang/lib/ASTMatchers/Dynamic/Diagnostics.cpp
  clang/lib/ASTMatchers/Dynamic/Parser.cpp
  clang/unittests/ASTMatchers/Dynamic/ParserTest.cpp

Index: clang/unittests/ASTMatchers/Dynamic/ParserTest.cpp
===
--- clang/unittests/ASTMatchers/Dynamic/ParserTest.cpp
+++ clang/unittests/ASTMatchers/Dynamic/ParserTest.cpp
@@ -32,6 +32,17 @@
 return M.getID().second;
   }
 
+  bool isBuilderMatcher(MatcherCtor) const override { return false; }
+
+  ASTNodeKind nodeMatcherType(MatcherCtor) const override { return {}; }
+
+  internal::MatcherDescriptorPtr
+  buildMatcherCtor(MatcherCtor, SourceRange NameRange,
+   ArrayRef Args,
+   Diagnostics *Error) const override {
+return internal::MatcherDescriptorPtr{nullptr};
+  }
+
   void parse(StringRef Code) {
 Diagnostics Error;
 VariantValue Value;
@@ -329,7 +340,7 @@
 "1:5: Invalid token <(> found when looking for a value.",
 ParseWithError("Foo(("));
   EXPECT_EQ("1:7: Expected end of code.", ParseWithError("expr()a"));
-  EXPECT_EQ("1:11: Malformed bind() expression.",
+  EXPECT_EQ("1:11: Period not followed by valid chained call.",
 ParseWithError("isArrow().biind"));
   EXPECT_EQ("1:15: Malformed bind() expression.",
 ParseWithError("isArrow().bind"));
@@ -340,6 +351,20 @@
   EXPECT_EQ("1:1: Error building matcher isArrow.\n"
 "1:1: Matcher does not support binding.",
 ParseWithError("isArrow().bind(\"foo\")"));
+  EXPECT_EQ("1:1: Error building matcher isArrow.\n"
+"1:11: Matcher does not support with call.",
+ParseWithError("isArrow().with"));
+  EXPECT_EQ(
+  "1:22: Error parsing matcher. Found token  while looking for '('.",
+  ParseWithError("mapAnyOf(ifStmt).with"));
+  EXPECT_EQ(
+  "1:22: Error parsing matcher. Found end-of-code while looking for ')'.",
+  ParseWithError("mapAnyOf(ifStmt).with("));
+  EXPECT_EQ("1:1: Failed to build matcher: mapAnyOf.",
+ParseWithError("mapAnyOf()"));
+  EXPECT_EQ("1:1: Error parsing argument 1 for matcher mapAnyOf.\n1:1: Failed "
+"to build matcher: mapAnyOf.",
+ParseWithError("mapAnyOf(\"foo\")"));
   EXPECT_EQ("Input value has unresolved overloaded type: "
 "Matcher",
 ParseMatcherWithError("hasBody(stmt())"));
@@ -470,7 +495,8 @@
 )matcher";
 M = Parser::parseMatcherExpression(Code, nullptr, , );
 EXPECT_FALSE(M.hasValue());
-EXPECT_EQ("1:11: Malformed bind() expression.", Error.toStringFull());
+EXPECT_EQ("1:11: Period not followed by valid chained call.",
+  Error.toStringFull());
   }
 
   {
Index: clang/lib/ASTMatchers/Dynamic/Parser.cpp
===
--- clang/lib/ASTMatchers/Dynamic/Parser.cpp
+++ clang/lib/ASTMatchers/Dynamic/Parser.cpp
@@ -52,6 +52,7 @@
 
   /// Some known identifiers.
   static const char* const ID_Bind;
+  static const char *const ID_With;
 
   TokenInfo() = default;
 
@@ -62,6 +63,7 @@
 };
 
 const char* const Parser::TokenInfo::ID_Bind = "bind";
+const char *const Parser::TokenInfo::ID_With = "with";
 
 /// Simple tokenizer for the parser.
 class Parser::CodeTokenizer {
@@ -367,14 +369,26 @@
 
   std::string BindID;
   Tokenizer->consumeNextToken();
-  TokenInfo BindToken = Tokenizer->consumeNextToken();
-  if (BindToken.Kind == TokenInfo::TK_CodeCompletion) {
-addCompletion(BindToken, MatcherCompletion("bind(\"", "bind", 1));
+  TokenInfo ChainCallToken = Tokenizer->consumeNextToken();
+  if (ChainCallToken.Kind == TokenInfo::TK_CodeCompletion) {
+addCompletion(ChainCallToken, MatcherCompletion("bind(\"", "bind", 1));
 return false;
   }
-  if (BindToken.Kind != TokenInfo::TK_Ident ||
-BindToken.Text != TokenInfo::ID_Bind) {
-Error->addError(BindToken.Range, Error->ET_ParserMalformedBindExpr);
+
+  if (ChainCallToken.Kind != TokenInfo::TK_Ident ||
+  (ChainCallToken.Text != TokenInfo::ID_Bind &&
+   ChainCallToken.Text != TokenInfo::ID_With)) {
+Error->addError(ChainCallToken.Range,
+Error->ET_ParserMalformedChainedExpr);
+return false;
+  }
+  if (ChainCallToken.Text == TokenInfo::ID_With) {
+
+Diagnostics::Context 

[PATCH] D94880: Add clang-query support for mapAnyOf

[Stephen Kelly via Phabricator via cfe-commits]

steveire added a comment.

In D94880#2544895 , @aaron.ballman 
wrote:

> In D94880#2544873 , @steveire wrote:
>
>> That page collapses the documentation for each matcher, as you know.
>>
>> You need to expand the docs for `mapAnyOf`, as you know.
>>
>> You seem to know all this. What's the remaining issue?
>
> The issue was that this was not at all obvious to me as a code reviewer, so I 
> was asking you for help. Your response comes across as patronizing, but thank 
> you for giving me the information I was looking for. FWIW, I did not know 
> this was under the `mapAnyOf` matcher and I was assuming this was going to be 
> documented at the top level, just like `bind()`.
>
> LG.

You reviewed the patches for `mapAnyOf` up to now and you reviewed this change 
which has unit tests using `with`. I didn't know what was unclear to you.  
Sorry for being terse.

Thanks for the review.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94880/new/

https://reviews.llvm.org/D94880

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D94880: Add clang-query support for mapAnyOf

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.

In D94880#2544873 , @steveire wrote:

> That page collapses the documentation for each matcher, as you know.
>
> You need to expand the docs for `mapAnyOf`, as you know.
>
> You seem to know all this. What's the remaining issue?

The issue was that this was not at all obvious to me as a code reviewer, so I 
was asking you for help. Your response comes across as patronizing, but thank 
you for giving me the information I was looking for. FWIW, I did not know this 
was under the `mapAnyOf` matcher and I was assuming this was going to be 
documented at the top level, just like `bind()`.

LG.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94880/new/

https://reviews.llvm.org/D94880

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D94880: Add clang-query support for mapAnyOf

[Stephen Kelly via Phabricator via cfe-commits]

steveire added a comment.

That page collapses the documentation for each matcher, as you know.

You need to expand the docs for `mapAnyOf`, as you know.

You seem to know all this. What's the remaining issue?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94880/new/

https://reviews.llvm.org/D94880

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D94880: Add clang-query support for mapAnyOf

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D94880#2544782 , @steveire wrote:

> @aaron.ballman Any further comments?

Thank you for pinging, this fell off my queue! The only further comments I have 
about about my confusion with `with`.

In D94880#2536485 , @steveire wrote:

> In D94880#2536399 , @aaron.ballman 
> wrote:
>
>> I think there should be some documentation change for the new `with` 
>> functionality.
>
> It's already documented in 
> https://clang.llvm.org/docs/LibASTMatchersReference.html , just like all the 
> other functionality.

Perhaps I'm blind, but when I search for "with" on that page, I get 21 hits and 
none of them are about the `with()` function. I am not finding evidence of 
`with` being supported on ToT, can you point me to the existing implementation? 
(If the functionality isn't being introduced in this patch, you don't need to 
document as part of this work. However, I was under the impression you were 
adding this functionality, so I'd like to understand this situation better 
before approving.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94880/new/

https://reviews.llvm.org/D94880

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D94880: Add clang-query support for mapAnyOf

[Stephen Kelly via Phabricator via cfe-commits]

steveire added a comment.

@aaron.ballman Any further comments?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94880/new/

https://reviews.llvm.org/D94880

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D94880: Add clang-query support for mapAnyOf

[Stephen Kelly via Phabricator via cfe-commits]

steveire added a comment.

In D94880#2536399 , @aaron.ballman 
wrote:

> I think there should be some documentation change for the new `with` 
> functionality.

It's already documented in 
https://clang.llvm.org/docs/LibASTMatchersReference.html , just like all the 
other functionality.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94880/new/

https://reviews.llvm.org/D94880

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D94880: Add clang-query support for mapAnyOf

[Stephen Kelly via Phabricator via cfe-commits]

steveire updated this revision to Diff 320774.
steveire added a comment.

Update


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94880/new/

https://reviews.llvm.org/D94880

Files:
  clang/include/clang/ASTMatchers/Dynamic/Diagnostics.h
  clang/include/clang/ASTMatchers/Dynamic/Parser.h
  clang/include/clang/ASTMatchers/Dynamic/Registry.h
  clang/include/clang/ASTMatchers/Dynamic/VariantValue.h
  clang/lib/ASTMatchers/Dynamic/Diagnostics.cpp
  clang/lib/ASTMatchers/Dynamic/Marshallers.h
  clang/lib/ASTMatchers/Dynamic/Parser.cpp
  clang/lib/ASTMatchers/Dynamic/Registry.cpp
  clang/lib/ASTMatchers/Dynamic/VariantValue.cpp
  clang/unittests/ASTMatchers/Dynamic/ParserTest.cpp

Index: clang/unittests/ASTMatchers/Dynamic/ParserTest.cpp
===
--- clang/unittests/ASTMatchers/Dynamic/ParserTest.cpp
+++ clang/unittests/ASTMatchers/Dynamic/ParserTest.cpp
@@ -32,6 +32,17 @@
 return M.getID().second;
   }
 
+  bool isBuilderMatcher(MatcherCtor) const override { return false; }
+
+  ASTNodeKind nodeMatcherType(MatcherCtor) const override { return {}; }
+
+  internal::MatcherDescriptorPtr
+  buildMatcherCtor(MatcherCtor, SourceRange NameRange,
+   ArrayRef Args,
+   Diagnostics *Error) const override {
+return internal::MatcherDescriptorPtr{nullptr};
+  }
+
   void parse(StringRef Code) {
 Diagnostics Error;
 VariantValue Value;
@@ -329,7 +340,7 @@
 "1:5: Invalid token <(> found when looking for a value.",
 ParseWithError("Foo(("));
   EXPECT_EQ("1:7: Expected end of code.", ParseWithError("expr()a"));
-  EXPECT_EQ("1:11: Malformed bind() expression.",
+  EXPECT_EQ("1:11: Period not followed by valid chained call.",
 ParseWithError("isArrow().biind"));
   EXPECT_EQ("1:15: Malformed bind() expression.",
 ParseWithError("isArrow().bind"));
@@ -340,6 +351,20 @@
   EXPECT_EQ("1:1: Error building matcher isArrow.\n"
 "1:1: Matcher does not support binding.",
 ParseWithError("isArrow().bind(\"foo\")"));
+  EXPECT_EQ("1:1: Error building matcher isArrow.\n"
+"1:11: Matcher does not support with call.",
+ParseWithError("isArrow().with"));
+  EXPECT_EQ(
+  "1:22: Error parsing matcher. Found token  while looking for '('.",
+  ParseWithError("mapAnyOf(ifStmt).with"));
+  EXPECT_EQ(
+  "1:22: Error parsing matcher. Found end-of-code while looking for ')'.",
+  ParseWithError("mapAnyOf(ifStmt).with("));
+  EXPECT_EQ("1:1: Failed to build matcher: mapAnyOf.",
+ParseWithError("mapAnyOf()"));
+  EXPECT_EQ("1:1: Error parsing argument 1 for matcher mapAnyOf.\n1:1: Failed "
+"to build matcher: mapAnyOf.",
+ParseWithError("mapAnyOf(\"foo\")"));
   EXPECT_EQ("Input value has unresolved overloaded type: "
 "Matcher",
 ParseMatcherWithError("hasBody(stmt())"));
@@ -470,7 +495,8 @@
 )matcher";
 M = Parser::parseMatcherExpression(Code, nullptr, , );
 EXPECT_FALSE(M.hasValue());
-EXPECT_EQ("1:11: Malformed bind() expression.", Error.toStringFull());
+EXPECT_EQ("1:11: Period not followed by valid chained call.",
+  Error.toStringFull());
   }
 
   {
@@ -515,6 +541,29 @@
   ASSERT_EQ(1u, Comps.size());
   EXPECT_EQ("bind(\"", Comps[0].TypedText);
   EXPECT_EQ("bind", Comps[0].MatcherDecl);
+
+  Code = "mapAny";
+  Comps = Parser::completeExpression(Code, 6);
+  ASSERT_EQ(1u, Comps.size());
+  EXPECT_EQ("Of(", Comps[0].TypedText);
+  EXPECT_EQ("Matcher "
+"mapAnyOf(NestedNameSpecifierLoc|QualType|TypeLoc|"
+"NestedNameSpecifier|Decl|Stmt|Type...)",
+Comps[0].MatcherDecl);
+
+  Code = "mapAnyOf(ifStmt).";
+  Comps = Parser::completeExpression(Code, 17);
+  ASSERT_EQ(2u, Comps.size());
+  EXPECT_EQ("bind(\"", Comps[0].TypedText);
+  EXPECT_EQ("bind", Comps[0].MatcherDecl);
+  EXPECT_EQ("with(\"", Comps[1].TypedText);
+  EXPECT_EQ("with", Comps[1].MatcherDecl);
+
+  Code = "mapAnyOf(ifS";
+  Comps = Parser::completeExpression(Code, 12);
+  ASSERT_EQ(1u, Comps.size());
+  EXPECT_EQ("tmt", Comps[0].TypedText);
+  EXPECT_EQ("ifStmt", Comps[0].MatcherDecl);
 }
 
 TEST(ParserTest, CompletionNamedValues) {
Index: clang/lib/ASTMatchers/Dynamic/VariantValue.cpp
===
--- clang/lib/ASTMatchers/Dynamic/VariantValue.cpp
+++ clang/lib/ASTMatchers/Dynamic/VariantValue.cpp
@@ -22,9 +22,11 @@
 std::string ArgKind::asString() const {
   switch (getArgKind()) {
   case AK_Matcher:
-return (Twine("Matcher<") + MatcherKind.asStringRef() + ">").str();
+return (Twine("Matcher<") + NodeKind.asStringRef() + ">").str();
   case AK_Boolean:
 return "boolean";
+  case AK_Node:
+return NodeKind.asStringRef().str();
   case AK_Double:
 return "double";
   case AK_Unsigned:
@@ -38,13 +40,13 @@
 bool 

[PATCH] D94880: Add clang-query support for mapAnyOf

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

I think there should be some documentation change for the new `with` 
functionality.




Comment at: clang/lib/ASTMatchers/Dynamic/Marshallers.h:398-407
+  bool isBuilderMatcher() const override { return false; }
+
+  std::unique_ptr
+  buildMatcherCtor(SourceRange, ArrayRef,
+   Diagnostics *) const override {
+return {};
+  }

Rather than repeat these degenerate definitions six times, should this just be 
the default implementation within `MatcherDescriptor` (so derived classes are 
not required to override them)? Alternatively, should we make a 
`NonBuilderMatcherDescriptor` class that holds the degenerate definition, and 
these derived classes can inherit from `NonBuilderMatcherDescriptor` instead?



Comment at: clang/lib/ASTMatchers/Dynamic/Marshallers.h:1094
+
+  void getArgKinds(ASTNodeKind ThisKind, unsigned ArgNo,
+   std::vector ) const override {





Comment at: clang/lib/ASTMatchers/Dynamic/Parser.cpp:373
+  Tokenizer->consumeNextToken(); // Consume the period.
+  const TokenInfo ChainCallToken = Tokenizer->consumeNextToken();
+  if (ChainCallToken.Kind == TokenInfo::TK_CodeCompletion) {





Comment at: clang/lib/ASTMatchers/Dynamic/Parser.cpp:449
 bool Parser::parseBindID(std::string ) {
-  // Parse .bind("foo")
-  assert(Tokenizer->peekNextToken().Kind == TokenInfo::TK_Period);
-  Tokenizer->consumeNextToken(); // consume the period.
-  const TokenInfo BindToken = Tokenizer->consumeNextToken();
-  if (BindToken.Kind == TokenInfo::TK_CodeCompletion) {
-addCompletion(BindToken, MatcherCompletion("bind(\"", "bind", 1));
-return false;
-  }
-
+  // Parse argument to .bind("foo")
   const TokenInfo OpenToken = Tokenizer->consumeNextToken();





Comment at: clang/lib/ASTMatchers/Dynamic/Parser.cpp:606
+
+  const TokenInfo WithOpenToken = Tokenizer->consumeNextToken();
+




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94880/new/

https://reviews.llvm.org/D94880

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D94880: Add clang-query support for mapAnyOf

[Stephen Kelly via Phabricator via cfe-commits]

steveire added inline comments.



Comment at: clang/lib/ASTMatchers/Dynamic/Registry.cpp:697
 
-  if (IsPolymorphic) {
-OS << "Matcher " << Name << "(Matcher";
+  std::string TypedText = std::string(Name);
+

aaron.ballman wrote:
> Any reason this declaration needed to move?
It's used after the if-else.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94880/new/

https://reviews.llvm.org/D94880

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D94880: Add clang-query support for mapAnyOf

[Stephen Kelly via Phabricator via cfe-commits]

steveire updated this revision to Diff 320593.
steveire added a comment.

Update


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94880/new/

https://reviews.llvm.org/D94880

Files:
  clang/include/clang/ASTMatchers/Dynamic/Diagnostics.h
  clang/include/clang/ASTMatchers/Dynamic/Parser.h
  clang/include/clang/ASTMatchers/Dynamic/Registry.h
  clang/include/clang/ASTMatchers/Dynamic/VariantValue.h
  clang/lib/ASTMatchers/Dynamic/Diagnostics.cpp
  clang/lib/ASTMatchers/Dynamic/Marshallers.h
  clang/lib/ASTMatchers/Dynamic/Parser.cpp
  clang/lib/ASTMatchers/Dynamic/Registry.cpp
  clang/lib/ASTMatchers/Dynamic/VariantValue.cpp
  clang/unittests/ASTMatchers/Dynamic/ParserTest.cpp

Index: clang/unittests/ASTMatchers/Dynamic/ParserTest.cpp
===
--- clang/unittests/ASTMatchers/Dynamic/ParserTest.cpp
+++ clang/unittests/ASTMatchers/Dynamic/ParserTest.cpp
@@ -32,6 +32,17 @@
 return M.getID().second;
   }
 
+  bool isBuilderMatcher(MatcherCtor) const override { return false; }
+
+  ASTNodeKind nodeMatcherType(MatcherCtor) const override { return {}; }
+
+  internal::MatcherDescriptorPtr
+  buildMatcherCtor(MatcherCtor, SourceRange NameRange,
+   ArrayRef Args,
+   Diagnostics *Error) const override {
+return internal::MatcherDescriptorPtr{nullptr};
+  }
+
   void parse(StringRef Code) {
 Diagnostics Error;
 VariantValue Value;
@@ -329,7 +340,7 @@
 "1:5: Invalid token <(> found when looking for a value.",
 ParseWithError("Foo(("));
   EXPECT_EQ("1:7: Expected end of code.", ParseWithError("expr()a"));
-  EXPECT_EQ("1:11: Malformed bind() expression.",
+  EXPECT_EQ("1:11: Period not followed by valid chained call.",
 ParseWithError("isArrow().biind"));
   EXPECT_EQ("1:15: Malformed bind() expression.",
 ParseWithError("isArrow().bind"));
@@ -340,6 +351,20 @@
   EXPECT_EQ("1:1: Error building matcher isArrow.\n"
 "1:1: Matcher does not support binding.",
 ParseWithError("isArrow().bind(\"foo\")"));
+  EXPECT_EQ("1:1: Error building matcher isArrow.\n"
+"1:11: Matcher does not support with call.",
+ParseWithError("isArrow().with"));
+  EXPECT_EQ(
+  "1:22: Error parsing matcher. Found token  while looking for '('.",
+  ParseWithError("mapAnyOf(ifStmt).with"));
+  EXPECT_EQ(
+  "1:22: Error parsing matcher. Found end-of-code while looking for ')'.",
+  ParseWithError("mapAnyOf(ifStmt).with("));
+  EXPECT_EQ("1:1: Failed to build matcher: mapAnyOf.",
+ParseWithError("mapAnyOf()"));
+  EXPECT_EQ("1:1: Error parsing argument 1 for matcher mapAnyOf.\n1:1: Failed "
+"to build matcher: mapAnyOf.",
+ParseWithError("mapAnyOf(\"foo\")"));
   EXPECT_EQ("Input value has unresolved overloaded type: "
 "Matcher",
 ParseMatcherWithError("hasBody(stmt())"));
@@ -470,7 +495,8 @@
 )matcher";
 M = Parser::parseMatcherExpression(Code, nullptr, , );
 EXPECT_FALSE(M.hasValue());
-EXPECT_EQ("1:11: Malformed bind() expression.", Error.toStringFull());
+EXPECT_EQ("1:11: Period not followed by valid chained call.",
+  Error.toStringFull());
   }
 
   {
@@ -515,6 +541,29 @@
   ASSERT_EQ(1u, Comps.size());
   EXPECT_EQ("bind(\"", Comps[0].TypedText);
   EXPECT_EQ("bind", Comps[0].MatcherDecl);
+
+  Code = "mapAny";
+  Comps = Parser::completeExpression(Code, 6);
+  ASSERT_EQ(1u, Comps.size());
+  EXPECT_EQ("Of(", Comps[0].TypedText);
+  EXPECT_EQ("Matcher "
+"mapAnyOf(NestedNameSpecifierLoc|QualType|TypeLoc|"
+"NestedNameSpecifier|Decl|Stmt|Type...)",
+Comps[0].MatcherDecl);
+
+  Code = "mapAnyOf(ifStmt).";
+  Comps = Parser::completeExpression(Code, 17);
+  ASSERT_EQ(2u, Comps.size());
+  EXPECT_EQ("bind(\"", Comps[0].TypedText);
+  EXPECT_EQ("bind", Comps[0].MatcherDecl);
+  EXPECT_EQ("with(\"", Comps[1].TypedText);
+  EXPECT_EQ("with", Comps[1].MatcherDecl);
+
+  Code = "mapAnyOf(ifS";
+  Comps = Parser::completeExpression(Code, 12);
+  ASSERT_EQ(1u, Comps.size());
+  EXPECT_EQ("tmt", Comps[0].TypedText);
+  EXPECT_EQ("ifStmt", Comps[0].MatcherDecl);
 }
 
 TEST(ParserTest, CompletionNamedValues) {
Index: clang/lib/ASTMatchers/Dynamic/VariantValue.cpp
===
--- clang/lib/ASTMatchers/Dynamic/VariantValue.cpp
+++ clang/lib/ASTMatchers/Dynamic/VariantValue.cpp
@@ -22,9 +22,11 @@
 std::string ArgKind::asString() const {
   switch (getArgKind()) {
   case AK_Matcher:
-return (Twine("Matcher<") + MatcherKind.asStringRef() + ">").str();
+return (Twine("Matcher<") + NodeKind.asStringRef() + ">").str();
   case AK_Boolean:
 return "boolean";
+  case AK_Node:
+return NodeKind.asStringRef().str();
   case AK_Double:
 return "double";
   case AK_Unsigned:
@@ -38,13 +40,13 @@
 bool 

[PATCH] D94880: Add clang-query support for mapAnyOf

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/ASTMatchers/Dynamic/Registry.h:36-37
 
+/// A smart (owning) pointer for MatcherDescriptor
+/// We can't use unique_ptr because MatcherDescriptor is forward declared
+class MatcherDescriptorPtr {





Comment at: clang/include/clang/ASTMatchers/Dynamic/Registry.h:40
+public:
+  MatcherDescriptorPtr(MatcherDescriptor * = nullptr);
+  ~MatcherDescriptorPtr();

I think it'd be cleaner to make callers be explicit about forming the smart 
pointer.



Comment at: clang/lib/ASTMatchers/Dynamic/Marshallers.h:586-587
+  std::unique_ptr
+  buildMatcherCtor(SourceRange NameRange, ArrayRef Args,
+   Diagnostics *Error) const override {
+return {};

Might as well make it obvious the function doesn't care about its arguments 
(same elsewhere in the patch).



Comment at: clang/lib/ASTMatchers/Dynamic/Marshallers.h:1081-1083
+if (NodeKinds.empty()) {
+  return {};
+}





Comment at: clang/lib/ASTMatchers/Dynamic/Parser.cpp:372
   std::string BindID;
-  if (!parseBindID(BindID))
+  Tokenizer->consumeNextToken(); // consume the period.
+  const TokenInfo ChainCallToken = Tokenizer->consumeNextToken();





Comment at: clang/lib/ASTMatchers/Dynamic/Parser.cpp:373
+  Tokenizer->consumeNextToken(); // consume the period.
+  const TokenInfo ChainCallToken = Tokenizer->consumeNextToken();
+  if (ChainCallToken.Kind == TokenInfo::TK_CodeCompletion) {





Comment at: clang/lib/ASTMatchers/Dynamic/Parser.cpp:435
+  assert(NameToken.Kind == TokenInfo::TK_Ident);
+  const TokenInfo OpenToken = Tokenizer->consumeNextToken();
+  if (OpenToken.Kind != TokenInfo::TK_OpenParen) {





Comment at: clang/lib/ASTMatchers/Dynamic/Parser.cpp:449
+bool Parser::parseBindID(TokenInfo BindToken, std::string ) {
   // Parse .bind("foo")
   const TokenInfo OpenToken = Tokenizer->consumeNextToken();

The comment is a bit misleading in that this doesn't parse the `.bind` part, 
only the `("foo")` bits.

Also, why does the function now accept `BindToken` but not use it?



Comment at: clang/lib/ASTMatchers/Dynamic/Parser.cpp:491
+// We must find a , token to continue.
+const TokenInfo CommaToken = Tokenizer->consumeNextToken();
+if (CommaToken.Kind != TokenInfo::TK_Comma) {





Comment at: clang/lib/ASTMatchers/Dynamic/Parser.cpp:510
+
+  const auto NodeMatcherToken = Tokenizer->consumeNextToken();
+





Comment at: clang/lib/ASTMatchers/Dynamic/Parser.cpp:521
+
+  auto MappedMatcher = S->lookupMatcherCtor(ArgValue.Text);
+





Comment at: clang/lib/ASTMatchers/Dynamic/Parser.cpp:553
+
+  auto BuiltCtor = S->buildMatcherCtor(Ctor, NameToken.Range, Args, Error);
+

unique_ptr? or is this our custom smart pointer? Either way, please spell the 
type out.



Comment at: clang/lib/ASTMatchers/Dynamic/Parser.cpp:563
+  if (Tokenizer->peekNextToken().Kind == TokenInfo::TK_Period) {
+Tokenizer->consumeNextToken(); // consume the period.
+const TokenInfo ChainCallToken = Tokenizer->consumeNextToken();





Comment at: clang/lib/ASTMatchers/Dynamic/Parser.cpp:638-641
+  if (Ctor && *Ctor && S->isBuilderMatcher(*Ctor)) {
+return parseMatcherBuilder(*Ctor, NameToken, OpenToken, Value);
+  }
+





Comment at: clang/lib/ASTMatchers/Dynamic/Parser.cpp:691
+Tokenizer->consumeNextToken();
+const TokenInfo ChainCallToken = Tokenizer->consumeNextToken();
+if (ChainCallToken.Kind == TokenInfo::TK_CodeCompletion) {





Comment at: clang/lib/ASTMatchers/Dynamic/Registry.cpp:580
+   ArrayRef Args, Diagnostics *Error) {
+  return Ctor->buildMatcherCtor(NameRange, Args, Error).release();
+}

I think that having the explicit ctor call here would make it more obvious that 
the `.release()` is being picked up by another smart pointer type.



Comment at: clang/lib/ASTMatchers/Dynamic/Registry.cpp:697
 
-  if (IsPolymorphic) {
-OS << "Matcher " << Name << "(Matcher";
+  std::string TypedText = std::string(Name);
+

Any reason this declaration needed to move?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94880/new/

https://reviews.llvm.org/D94880

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D94880: Add clang-query support for mapAnyOf

[Stephen Kelly via Phabricator via cfe-commits]

steveire added a comment.

@aaron.ballman Please review this again as I've merged some more needed changes 
into it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94880/new/

https://reviews.llvm.org/D94880

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D94880: Add clang-query support for mapAnyOf

[Stephen Kelly via Phabricator via cfe-commits]

steveire updated this revision to Diff 319431.
steveire added a comment.

Update


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94880/new/

https://reviews.llvm.org/D94880

Files:
  clang/include/clang/ASTMatchers/Dynamic/Diagnostics.h
  clang/include/clang/ASTMatchers/Dynamic/Parser.h
  clang/include/clang/ASTMatchers/Dynamic/Registry.h
  clang/include/clang/ASTMatchers/Dynamic/VariantValue.h
  clang/lib/ASTMatchers/Dynamic/Diagnostics.cpp
  clang/lib/ASTMatchers/Dynamic/Marshallers.h
  clang/lib/ASTMatchers/Dynamic/Parser.cpp
  clang/lib/ASTMatchers/Dynamic/Registry.cpp
  clang/lib/ASTMatchers/Dynamic/VariantValue.cpp
  clang/unittests/ASTMatchers/Dynamic/ParserTest.cpp

Index: clang/unittests/ASTMatchers/Dynamic/ParserTest.cpp
===
--- clang/unittests/ASTMatchers/Dynamic/ParserTest.cpp
+++ clang/unittests/ASTMatchers/Dynamic/ParserTest.cpp
@@ -32,6 +32,17 @@
 return M.getID().second;
   }
 
+  bool isBuilderMatcher(MatcherCtor) const override { return false; }
+
+  ASTNodeKind nodeMatcherType(MatcherCtor) const override { return {}; }
+
+  internal::MatcherDescriptorPtr
+  buildMatcherCtor(MatcherCtor, SourceRange NameRange,
+   ArrayRef Args,
+   Diagnostics *Error) const override {
+return {};
+  }
+
   void parse(StringRef Code) {
 Diagnostics Error;
 VariantValue Value;
@@ -329,7 +340,7 @@
 "1:5: Invalid token <(> found when looking for a value.",
 ParseWithError("Foo(("));
   EXPECT_EQ("1:7: Expected end of code.", ParseWithError("expr()a"));
-  EXPECT_EQ("1:11: Malformed bind() expression.",
+  EXPECT_EQ("1:11: Period not followed by valid chained call.",
 ParseWithError("isArrow().biind"));
   EXPECT_EQ("1:15: Malformed bind() expression.",
 ParseWithError("isArrow().bind"));
@@ -340,6 +351,20 @@
   EXPECT_EQ("1:1: Error building matcher isArrow.\n"
 "1:1: Matcher does not support binding.",
 ParseWithError("isArrow().bind(\"foo\")"));
+  EXPECT_EQ("1:1: Error building matcher isArrow.\n"
+"1:11: Matcher does not support with call.",
+ParseWithError("isArrow().with"));
+  EXPECT_EQ(
+  "1:22: Error parsing matcher. Found token  while looking for '('.",
+  ParseWithError("mapAnyOf(ifStmt).with"));
+  EXPECT_EQ(
+  "1:22: Error parsing matcher. Found end-of-code while looking for ')'.",
+  ParseWithError("mapAnyOf(ifStmt).with("));
+  EXPECT_EQ("1:1: Failed to build matcher: mapAnyOf.",
+ParseWithError("mapAnyOf()"));
+  EXPECT_EQ("1:1: Error parsing argument 1 for matcher mapAnyOf.\n1:1: Failed "
+"to build matcher: mapAnyOf.",
+ParseWithError("mapAnyOf(\"foo\")"));
   EXPECT_EQ("Input value has unresolved overloaded type: "
 "Matcher",
 ParseMatcherWithError("hasBody(stmt())"));
@@ -470,7 +495,8 @@
 )matcher";
 M = Parser::parseMatcherExpression(Code, nullptr, , );
 EXPECT_FALSE(M.hasValue());
-EXPECT_EQ("1:11: Malformed bind() expression.", Error.toStringFull());
+EXPECT_EQ("1:11: Period not followed by valid chained call.",
+  Error.toStringFull());
   }
 
   {
@@ -515,6 +541,29 @@
   ASSERT_EQ(1u, Comps.size());
   EXPECT_EQ("bind(\"", Comps[0].TypedText);
   EXPECT_EQ("bind", Comps[0].MatcherDecl);
+
+  Code = "mapAny";
+  Comps = Parser::completeExpression(Code, 6);
+  ASSERT_EQ(1u, Comps.size());
+  EXPECT_EQ("Of(", Comps[0].TypedText);
+  EXPECT_EQ("Matcher "
+"mapAnyOf(NestedNameSpecifierLoc|QualType|TypeLoc|"
+"NestedNameSpecifier|Decl|Stmt|Type...)",
+Comps[0].MatcherDecl);
+
+  Code = "mapAnyOf(ifStmt).";
+  Comps = Parser::completeExpression(Code, 17);
+  ASSERT_EQ(2u, Comps.size());
+  EXPECT_EQ("bind(\"", Comps[0].TypedText);
+  EXPECT_EQ("bind", Comps[0].MatcherDecl);
+  EXPECT_EQ("with(\"", Comps[1].TypedText);
+  EXPECT_EQ("with", Comps[1].MatcherDecl);
+
+  Code = "mapAnyOf(ifS";
+  Comps = Parser::completeExpression(Code, 12);
+  ASSERT_EQ(1u, Comps.size());
+  EXPECT_EQ("tmt", Comps[0].TypedText);
+  EXPECT_EQ("ifStmt", Comps[0].MatcherDecl);
 }
 
 TEST(ParserTest, CompletionNamedValues) {
Index: clang/lib/ASTMatchers/Dynamic/VariantValue.cpp
===
--- clang/lib/ASTMatchers/Dynamic/VariantValue.cpp
+++ clang/lib/ASTMatchers/Dynamic/VariantValue.cpp
@@ -22,9 +22,11 @@
 std::string ArgKind::asString() const {
   switch (getArgKind()) {
   case AK_Matcher:
-return (Twine("Matcher<") + MatcherKind.asStringRef() + ">").str();
+return (Twine("Matcher<") + NodeKind.asStringRef() + ">").str();
   case AK_Boolean:
 return "boolean";
+  case AK_Node:
+return NodeKind.asStringRef().str();
   case AK_Double:
 return "double";
   case AK_Unsigned:
@@ -38,13 +40,13 @@
 bool ArgKind::isConvertibleTo(ArgKind To, unsigned 

[PATCH] D94880: Add clang-query support for mapAnyOf

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM aside from some minor nits.




Comment at: clang/lib/ASTMatchers/Dynamic/Marshallers.h:1081
+if (NodeKinds.empty()) {
+  // Set error
+  return {};

Is this a TODO comment?



Comment at: clang/lib/ASTMatchers/Dynamic/Marshallers.h:1095-1098
+  void getArgKinds(ASTNodeKind ThisKind, unsigned ArgNo,
+   std::vector ) const override {
+return;
+  }





Comment at: clang/lib/ASTMatchers/Dynamic/Marshallers.h:1099-1100
+  }
+  bool isConvertibleTo(ASTNodeKind Kind, unsigned *Specificity = nullptr,
+   ASTNodeKind *LeastDerivedKind = nullptr) const override 
{
+return false;




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94880/new/

https://reviews.llvm.org/D94880

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D94880: Add clang-query support for mapAnyOf

[Stephen Kelly via Phabricator via cfe-commits]

steveire created this revision.
steveire added a reviewer: aaron.ballman.
steveire requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D94880

Files:
  clang/lib/ASTMatchers/Dynamic/Marshallers.h
  clang/lib/ASTMatchers/Dynamic/Registry.cpp


Index: clang/lib/ASTMatchers/Dynamic/Registry.cpp
===
--- clang/lib/ASTMatchers/Dynamic/Registry.cpp
+++ clang/lib/ASTMatchers/Dynamic/Registry.cpp
@@ -102,6 +102,9 @@
   // Other:
   // equalsNode
 
+  registerMatcher("mapAnyOf",
+  std::make_unique());
+
   REGISTER_OVERLOADED_2(callee);
   REGISTER_OVERLOADED_2(hasAnyCapture);
   REGISTER_OVERLOADED_2(hasPrefix);
Index: clang/lib/ASTMatchers/Dynamic/Marshallers.h
===
--- clang/lib/ASTMatchers/Dynamic/Marshallers.h
+++ clang/lib/ASTMatchers/Dynamic/Marshallers.h
@@ -1057,6 +1057,55 @@
   ASTNodeKind nodeMatcherType() const override { return ASTNodeKind(); }
 };
 
+class MapAnyOfBuilderDescriptor : public MatcherDescriptor {
+public:
+  VariantMatcher create(SourceRange NameRange, ArrayRef Args,
+Diagnostics *Error) const override {
+return {};
+  }
+
+  bool isBuilderMatcher() const override { return true; }
+
+  std::unique_ptr
+  buildMatcherCtor(SourceRange NameRange, ArrayRef Args,
+   Diagnostics *Error) const override {
+
+std::vector NodeKinds;
+for (auto Arg : Args) {
+  if (!Arg.Value.isNodeKind())
+return {};
+  NodeKinds.push_back(Arg.Value.getNodeKind());
+}
+
+if (NodeKinds.empty()) {
+  // Set error
+  return {};
+}
+
+ASTNodeKind CladeNodeKind = NodeKinds.front().getCladeKind();
+
+return std::make_unique(CladeNodeKind,
+   NodeKinds);
+  }
+
+  bool isVariadic() const override { return false; }
+
+  unsigned getNumArgs() const override { return 0; }
+
+  void getArgKinds(ASTNodeKind ThisKind, unsigned ArgNo,
+   std::vector ) const override {
+return;
+  }
+  bool isConvertibleTo(ASTNodeKind Kind, unsigned *Specificity = nullptr,
+   ASTNodeKind *LeastDerivedKind = nullptr) const override 
{
+return false;
+  }
+
+  bool isPolymorphic() const override { return false; }
+
+  ASTNodeKind nodeMatcherType() const override { return ASTNodeKind(); }
+};
+
 /// Helper functions to select the appropriate marshaller functions.
 /// They detect the number of arguments, arguments types and return type.
 


Index: clang/lib/ASTMatchers/Dynamic/Registry.cpp
===
--- clang/lib/ASTMatchers/Dynamic/Registry.cpp
+++ clang/lib/ASTMatchers/Dynamic/Registry.cpp
@@ -102,6 +102,9 @@
   // Other:
   // equalsNode
 
+  registerMatcher("mapAnyOf",
+  std::make_unique());
+
   REGISTER_OVERLOADED_2(callee);
   REGISTER_OVERLOADED_2(hasAnyCapture);
   REGISTER_OVERLOADED_2(hasPrefix);
Index: clang/lib/ASTMatchers/Dynamic/Marshallers.h
===
--- clang/lib/ASTMatchers/Dynamic/Marshallers.h
+++ clang/lib/ASTMatchers/Dynamic/Marshallers.h
@@ -1057,6 +1057,55 @@
   ASTNodeKind nodeMatcherType() const override { return ASTNodeKind(); }
 };
 
+class MapAnyOfBuilderDescriptor : public MatcherDescriptor {
+public:
+  VariantMatcher create(SourceRange NameRange, ArrayRef Args,
+Diagnostics *Error) const override {
+return {};
+  }
+
+  bool isBuilderMatcher() const override { return true; }
+
+  std::unique_ptr
+  buildMatcherCtor(SourceRange NameRange, ArrayRef Args,
+   Diagnostics *Error) const override {
+
+std::vector NodeKinds;
+for (auto Arg : Args) {
+  if (!Arg.Value.isNodeKind())
+return {};
+  NodeKinds.push_back(Arg.Value.getNodeKind());
+}
+
+if (NodeKinds.empty()) {
+  // Set error
+  return {};
+}
+
+ASTNodeKind CladeNodeKind = NodeKinds.front().getCladeKind();
+
+return std::make_unique(CladeNodeKind,
+   NodeKinds);
+  }
+
+  bool isVariadic() const override { return false; }
+
+  unsigned getNumArgs() const override { return 0; }
+
+  void getArgKinds(ASTNodeKind ThisKind, unsigned ArgNo,
+   std::vector ) const override {
+return;
+  }
+  bool isConvertibleTo(ASTNodeKind Kind, unsigned *Specificity = nullptr,
+   ASTNodeKind *LeastDerivedKind = nullptr) const override {
+return false;
+  }
+
+  bool isPolymorphic() const override { return false; }
+
+  ASTNodeKind nodeMatcherType() const override { return ASTNodeKind(); }
+};
+
 /// Helper functions to select the appropriate marshaller functions.
 /// They detect the number of arguments,