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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
