[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<MatcherDescriptor>
+  buildMatcherCtor(SourceRange NameRange, ArrayRef<ParserValue> 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 &BindID) {
   // 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<ParserValue> 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<T> " << Name << "(Matcher<T>";
+      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