[PATCH] D54405: Record whether a AST Matcher constructs a Node

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: lib/ASTMatchers/Dynamic/Registry.cpp:70
+  void registerIfNodeMatcher(MatcherType,
+                             internal::MatcherDescriptor *matchDescriptor,
+                             StringRef) {}
----------------
Fix `matchDescriptor` for coding conventions, but please don't reuse the name of a type when fixing it. :-)


================
Comment at: lib/ASTMatchers/Dynamic/Registry.cpp:76
+      ast_matchers::internal::VariadicAllOfMatcher<ResultT> VarFunc,
+      internal::MatcherDescriptor *matchDescriptor, StringRef MatcherName) {
+    auto K = ast_type_traits::ASTNodeKind::getFromNodeKind<
----------------
Same comment here.


================
Comment at: lib/ASTMatchers/Dynamic/Registry.cpp:77
+      internal::MatcherDescriptor *matchDescriptor, StringRef MatcherName) {
+    auto K = ast_type_traits::ASTNodeKind::getFromNodeKind<
+        typename ast_matchers::internal::VariadicAllOfMatcher<ResultT>::Type>();
----------------
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.


================
Comment at: lib/ASTMatchers/Dynamic/Registry.cpp:79
+        typename ast_matchers::internal::VariadicAllOfMatcher<ResultT>::Type>();
+    if (!K.isNone()) {
+      NodeConstructors[K] = std::make_pair(MatcherName, matchDescriptor);
----------------
Elide braces.


================
Comment at: lib/ASTMatchers/Dynamic/Registry.cpp:88
+          VarFunc,
+      internal::MatcherDescriptor *matchDescriptor, StringRef MatcherName) {
+    auto K = ast_type_traits::ASTNodeKind::getFromNodeKind<DerivedT>();
----------------
Same `matchDescriptor` here.


================
Comment at: lib/ASTMatchers/Dynamic/Registry.cpp:89
+      internal::MatcherDescriptor *matchDescriptor, StringRef MatcherName) {
+    auto K = ast_type_traits::ASTNodeKind::getFromNodeKind<DerivedT>();
+    if (!K.isNone()) {
----------------
Similar comment about `auto` here.


================
Comment at: lib/ASTMatchers/Dynamic/Registry.cpp:90
+    auto K = ast_type_traits::ASTNodeKind::getFromNodeKind<DerivedT>();
+    if (!K.isNone()) {
+      NodeConstructors[K] = std::make_pair(MatcherName, matchDescriptor);
----------------
Elide braces.


Repository:
  rC Clang

https://reviews.llvm.org/D54405