Record whether a AST Matcher constructs a Node

[steveire]

Summary

These matchers are bindable. Recording this information will make it
possible to introspect the matchers which can be used inside another
matcher.

[aaron.ballman]

lib/ASTMatchers/Dynamic/Registry.cpp

70

Fix matchDescriptor for coding conventions, but please don't reuse the name of a type when fixing it. :-)

76

Same comment here.

77

Mildly not keen on the use of auto here. It's a factory function, so it kind of names the resulting type, but it also looks like the type will be ast_matchers::internal::VariadicAllOfMatcher<ResultT>::Type from the template argument, which is incorrect.

79

Elide braces.

88

Same matchDescriptor here.

89

Similar comment about auto here.

90

Elide braces.

[steveire]

lib/ASTMatchers/Dynamic/Registry.cpp

77

There is no reason to assume that taking a template argument means that type is result.

The method is getFrom which decreases the ambiguity still further.

Spelling out the type doesn't add anything useful. This should be ok.

[steveire]

Update

[aaron.ballman]

Aside from the uses of auto and the lack of tests, this LGTM.

lib/ASTMatchers/Dynamic/Registry.cpp

77

There is no reason to assume that taking a template argument means that type is result.

Aside from all the other places that do exactly that (getAs<>, cast<>, dyn_cast<>, castAs<>, and so on). Generally, when we have a function named get that takes a template type argument, the result when seen in proximity to auto is the template type.

Spelling out the type doesn't add anything useful. This should be ok.

I slept on this one and fall on the other side of it; using auto hides information that tripped up at least one person when reading the code, so don't use auto. It's not clear enough what the resulting type will be.

[steveire]

lib/ASTMatchers/Dynamic/Registry.cpp

77

I put this in the category of requiring

SomeType ST = SomeType::create();

instead of

auto ST = SomeType::create();

ast_type_traits::ASTNodeKind is already on that line and you want to see it again.

[zturner]

lib/ASTMatchers/Dynamic/Registry.cpp

77

FWIW I'm with Aaron here. Im' not familiar at all with this codebase, and looking at the code, my first instinct is "the result type is probably ast_matchers::internal::VariadicAllOfMatcher<ResultT>::Type". According to Aaron's earlier comment, that is incorrect, so there's at least 1 data point that it hinders readability.

So, honest question. What *is* the return type here?

[aaron.ballman]

lib/ASTMatchers/Dynamic/Registry.cpp

77

So, honest question. What *is* the return type here?

Much to my surprise, it's ASTNodeKind. It was entirely unobvious to me that this was a factory function.

[steveire]

lib/ASTMatchers/Dynamic/Registry.cpp

77

@zturner Quoting myself:

ast_type_traits::ASTNodeKind is already on that line and you want to see it again.

The expression is getFromNodeKind. There is a pattern of SomeType::fromFooKind<FooKind>() returning a SomeType. This is not so unusual.

[zturner]

lib/ASTMatchers/Dynamic/Registry.cpp

77

Note that at the top of this file there's already a using namespace clang::ast_type_traits; So if you're worried about verbosity, then you can remove the ast_type_traits::, remove the auto, and the net effect is that the overall line will end up being shorter.

[steveire]

Update

[steveire]

lib/ASTMatchers/Dynamic/Registry.cpp

77

The funny thing is - if you see a line like

Parser CodeParser = Parser::getFromArgs(Args);

you have no idea what type Parser is!

To start, it could be clang::Parser or clang::ast_matchers::dynamic::Parser, depending on a using namespace which might appear any distance up in the file. It is not uncommon for clang files to be thousands of lines lone.

Or it could be inside a template with template<typename Parser>, or there could be a using Parser = Foo; any distance up in the file.

Parser CodeParser = Parser::getFromArgs(Args);

is no more helpful than

auto CodeParser = Parser::getFromArgs(Args);

Sticking with the same example, if CodeParser is a field, then you might have a line

CodeParser = Parser::getFromArgs(Args);

and you could object and create a macro which expands to nothing to ensure that the type appears on the line

CodeParser = CALL_RETURNS(Parser)Parser::getFromArgs(Args);

No one does that, because it is ok for the type to not appear on the line.

You would also have to object to code such as

Object.setParser(Parser::getFromArgs(Args));

requiring it to instead be

Parser CodeParser = Parser::getFromArgs(Args);
Object.setParser(CodeParser);

so that you can read the type.

Even then, what if those two lines are separated by a full page of code? How do you know the type of CodeParser in the Object.setParser(CodeParser) call? The answer is you don't and you don't need to.

You would also require

return Parser::getFromArgs(Args);

to instead be

Parser CodeParser = Parser::getFromArgs(Args);
return CodeParser;

Claims that human readers need to see a type are as incorrect as they are inconsistent.

[cfe-commits]

I don’t really have much more to add here except to refer you to the style
guide:

https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable

Specifically this line: “Use auto if and only if it makes the code more
readable or easier to maintain.”

Given that 2 out of 2 reviewers who have looked at this have said it did
not make the code more readable or easier to maintain for them , and have
further said that they feel the return type is not “obvious from the
context”, i do not believe this usage fits within our style guidelines.

I don’t think it’s worth beating on this anymore though, because this is a
lot of back and forth over something that doesn’t actually merit this much
discussion. So in the interest of conforming to the style guide above,
please remove auto and then start a thread on llvm-dev if you disagree with
the policy

• msg-532-122.txt159 BDownload

[steveire]

Refactor

[steveire]

lib/ASTMatchers/Dynamic/Registry.cpp

85

You still can't see the return type of getFromNodeKind() here, but I trust that this is fine nonetheless.

[aaron.ballman]

LGTM! Please commit with whatever patch makes use of nodeConstructors() (as this functionality doesn't stand on its own).

include/clang/ASTMatchers/ASTMatchersInternal.h

 /// \c Matcher<NestedNameSpecifier> given a variable number of arguments of type
 /// \c Matcher<NestedNameSpecifier>.
 /// The returned matcher matches if all given matchers match.
 template <typename T>
 class VariadicAllOfMatcher
     : public VariadicFunction<BindableMatcher<T>, Matcher<T>,
                               makeAllOfComposite<T>> {
 public:
+  using Type = T;
   VariadicAllOfMatcher() {}
 };
 
 /// Matches nodes of type \c TLoc for which the inner
 /// \c Matcher<T> matches.
 template <typename TLoc, typename T>
 class LocMatcher : public WrapperMatcherInterface<TLoc> {
 public:

lib/ASTMatchers/Dynamic/Registry.cpp

 namespace dynamic {
 
 namespace {
 
 using internal::MatcherDescriptor;
 
 using ConstructorMap = llvm::StringMap<std::unique_ptr<const MatcherDescriptor>>;
 
+using NodeConstructorMap =
+    std::map<ast_type_traits::ASTNodeKind,
+             std::pair<std::string, const internal::MatcherDescriptor *>>;
+
 class RegistryMaps {
 public:
   RegistryMaps();
   ~RegistryMaps();
 
   const ConstructorMap &constructors() const { return Constructors; }
+  const NodeConstructorMap &nodeConstructors() const {
+    return NodeConstructors;
+  }
+
+  void registerNodeMatcher(ast_type_traits::ASTNodeKind kind,
+                           internal::MatcherDescriptor *Callback);
 
 private:
   void registerMatcher(StringRef MatcherName,
                        std::unique_ptr<MatcherDescriptor> Callback);
 
+  template <typename MatcherType>
+  void registerIfNodeMatcher(MatcherType,
+                             internal::MatcherDescriptor *Descriptor,
+                             StringRef) {}
+
+  template <typename ResultT>
+  void registerIfNodeMatcher(
+      ast_matchers::internal::VariadicAllOfMatcher<ResultT> VarFunc,
+      internal::MatcherDescriptor *Descriptor, StringRef MatcherName) {
+    auto NodeKind = ast_type_traits::ASTNodeKind::getFromNodeKind<
+        typename ast_matchers::internal::VariadicAllOfMatcher<ResultT>::Type>();
+    if (!NodeKind.isNone())
+      NodeConstructors[NodeKind] = std::make_pair(MatcherName, Descriptor);
+  }
+
+  template <typename BaseT, typename DerivedT>
+  void registerIfNodeMatcher(
+      ast_matchers::internal::VariadicDynCastAllOfMatcher<BaseT, DerivedT>
+          VarFunc,
+      internal::MatcherDescriptor *Descriptor, StringRef MatcherName) {
+    auto NodeKind = ast_type_traits::ASTNodeKind::getFromNodeKind<DerivedT>();
+    if (!NodeKind.isNone())
+      NodeConstructors[NodeKind] = std::make_pair(MatcherName, Descriptor);
+  }
+
   ConstructorMap Constructors;
+  NodeConstructorMap NodeConstructors;
 };
 
 } // namespace
 
 void RegistryMaps::registerMatcher(
     StringRef MatcherName, std::unique_ptr<MatcherDescriptor> Callback) {
   assert(Constructors.find(MatcherName) == Constructors.end());
   Constructors[MatcherName] = std::move(Callback);
 }
 
 #define REGISTER_MATCHER(name)                                                 \
-  registerMatcher(#name, internal::makeMatcherAutoMarshall(                    \
-                             ::clang::ast_matchers::name, #name));
+  {                                                                            \
+    auto Descriptor =                                                          \
+        internal::makeMatcherAutoMarshall(::clang::ast_matchers::name, #name); \
+    registerIfNodeMatcher(::clang::ast_matchers::name, Descriptor.get(),       \
+                          #name);                                              \
+    registerMatcher(#name, std::move(Descriptor));                             \
+  }
 
 #define REGISTER_MATCHER_OVERLOAD(name)                                        \
   registerMatcher(#name,                                                       \
       llvm::make_unique<internal::OverloadedMatcherDescriptor>(name##Callbacks))
 
 #define SPECIFIC_MATCHER_OVERLOAD(name, Id)                                    \
   static_cast<::clang::ast_matchers::name##_Type##Id>(                         \
       ::clang::ast_matchers::name)