[PATCH] D54408: Add matchers available through casting to derived

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: lib/ASTMatchers/Dynamic/Registry.cpp:645
+getNodeConstructorType(MatcherCtor targetCtor) {
+  auto const &ctors = RegistryData->nodeConstructors();
+
----------------
Don't use `auto` here (and if you did. the `const` would go on the other side).


================
Comment at: lib/ASTMatchers/Dynamic/Registry.cpp:647-651
+  for (auto ctor : ctors) {
+    if (ctor.second.second == targetCtor)
+      return std::make_pair(ctor.first, ctor.second.first);
+  }
+  return llvm::None;
----------------
I think this can be replaced with `llvm::find_if()`.


================
Comment at: lib/ASTMatchers/Dynamic/Registry.cpp:659
+getMatchingMatchersImpl(ast_type_traits::ASTNodeKind StaticType,
+                        bool ExactOnly = false) {
+
----------------
I'd prefer this to not be a default argument (the callers can pass it in). It might also be nice to use an enum here rather than a bool, but alternative to that, you should stick in comments in the callers that describe the parameter name. e.g. `getMatchingMatchersImpl(yada, /*ExactOnly*/true);`


================
Comment at: lib/ASTMatchers/Dynamic/Registry.cpp:711
+          if (ExactOnly) {
+            auto IsConvertible = Matcher.isConvertibleTo(
+                StaticType, &Specificity, &LeastDerivedKind);
----------------
Don't use `auto`.


================
Comment at: lib/ASTMatchers/Dynamic/Registry.cpp:716-718
+        }
+
+        {
----------------
Why do these need their own inner block scopes?


================
Comment at: lib/ASTMatchers/Dynamic/Registry.cpp:719
+        {
+          auto TypeForMatcherOpt = getNodeConstructorType(&Matcher);
+          if (TypeForMatcherOpt) {
----------------
`auto`.


================
Comment at: lib/ASTMatchers/Dynamic/Registry.cpp:720-721
+          auto TypeForMatcherOpt = getNodeConstructorType(&Matcher);
+          if (TypeForMatcherOpt) {
+            if (StaticType.isBaseOf(TypeForMatcherOpt->first)) {
+              auto DerivedResults =
----------------
`if (TypeForMatcherOpt && StaticType.isBaseOf(...)) {`


================
Comment at: lib/ASTMatchers/Dynamic/Registry.cpp:722-723
+            if (StaticType.isBaseOf(TypeForMatcherOpt->first)) {
+              auto DerivedResults =
+                  getDerivedResults(TypeForMatcherOpt->first, Name);
+              Result.insert(Result.end(), DerivedResults.begin(),
----------------
`auto`, here and everywhere in this file.


================
Comment at: lib/ASTMatchers/Dynamic/Registry.cpp:749
+  for (auto item : NestedResult) {
+
+    auto nestedMatcherName = item.MatcherString;
----------------
Can remove whitespace around here.


================
Comment at: lib/ASTMatchers/Dynamic/Registry.cpp:761-766
+Registry::getMatchingMatchers(ast_type_traits::ASTNodeKind StaticType) {
+
+  std::vector<MatchingMatcher> Result = getMatchingMatchersImpl(StaticType);
+
+  return Result;
+}
----------------
It seems like `getMatchingMatchersImpl()` should just be renamed to `getMatchingMatchers()`.


Repository:
  rC Clang

https://reviews.llvm.org/D54408