aaron.ballman added inline comments.

================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:3553
+/// \endcode
+AST_MATCHER_P(CXXBaseSpecifier, hasClass, internal::Matcher<CXXRecordDecl>,
+              InnerMatcher) {
----------------
njames93 wrote:
> jkorous wrote:
> > aaron.ballman wrote:
> > > jkorous wrote:
> > > > Nit: while "[base specifier] `hasType`" sounds natural to me for some 
> > > > reason `hasClass` doesn't. English is not my first language though.
> > > I agree that `hasClass` seems unnatural here. Out of curiosity, could we 
> > > modify the `hasName` matcher to work on base specifiers so you can write: 
> > > `cxxRecordDecl(hasAnyBase(hasName("Base")))` as shorthand for the more 
> > > wordy version 
> > > `cxxRecordDecl(hasAnyBase(hasType(cxxRecordDecl(hasName("Base")))))`?
> > Wouldn't it be strange to treat `hasName` differently than all the other 
> > narrowing matchers? Honest question - I feel that `hasName` might be the 
> > most commonly used, just don't know if that's enough to justify this.
> > https://clang.llvm.org/docs/LibASTMatchersReference.html#narrowing-matchers
> Repurposing `hasName` would be a pain especially considering 99% of its use 
> cases wont be for base class matching.
> Wouldn't it be strange to treat hasName differently than all the other 
> narrowing matchers? Honest question - I feel that hasName might be the most 
> commonly used, just don't know if that's enough to justify this. 
> https://clang.llvm.org/docs/LibASTMatchersReference.html#narrowing-matchers

Different how? I'm suggesting to overload `hasName` to work on a 
`CXXBaseSpecifier` since those have a name.

> Repurposing hasName would be a pain especially considering 99% of its use 
> cases wont be for base class matching.

I'm asking what the right API is for users, though, which is a bit different. 
Base specifiers have names (there are no unnamed base specifiers), so to me, it 
makes more sense for `hasName` to work with them directly since that is the 
thing that does name matching.

I think you can accomplish this by using a `PolymorphicMatcherWithParam1` like 
we do for `hasOverloadedOperatorName` which can narrow to either a 
`CXXOperatorCallExpr` or a `FunctionDecl`.


================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:3621
+/// \endcode
+AST_MATCHER_P(CXXBaseSpecifier, hasClassOrClassTemplate,
+              internal::Matcher<CXXRecordDecl>, InnerMatcher) {
----------------
jkorous wrote:
> I think we should just use `eachOf` matcher for this kind of composition.
> 
> https://clang.llvm.org/docs/LibASTMatchersReference.html#traversal-matchers
+1


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81552/new/

https://reviews.llvm.org/D81552



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to