Page MenuHomePhabricator

[ASTMatchers] Added hasDirectBase Matcher
ClosedPublic

Authored by njames93 on Jun 10 2020, 4:55 AM.

Details

Summary

Adds a matcher called hasDirectBase for matching the CXXBaseSpecifier of a class that directly derives from another class.

Diff Detail

Event Timeline

njames93 created this revision.Jun 10 2020, 4:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 10 2020, 4:55 AM
aaron.ballman added inline comments.Jun 10 2020, 7:31 AM
clang/include/clang/ASTMatchers/ASTMatchers.h
2886–2888

It seems like these aren't really part of the example?

3541

This is undoing a change that was just added less than two weeks ago, so I think the potential for breaking code is small. That said, can you explain why you think hasClass is a better approach than hasType?

njames93 marked 2 inline comments as done.Jun 10 2020, 7:55 AM
njames93 added inline comments.
clang/include/clang/ASTMatchers/ASTMatchers.h
2886–2888

They are, just not directly. Shows it won't match any old base specifier.

3541

Yeah, as that change hasn't reached landed onto a release branch breaking code shouldn't be an issue, If it was I'd leave it in.

  • hasType is very generic, whereas hasClass is specific to what a CXXBaseSpecifier supports.
  • It makes the matchers marginally simpler. hasDirectBase(hasType(cxxRecordDecl(hasName("Base")))) vs hasDirectBase(hasClass(hasName("Base")))
  • In the documentation it also specifies that hasClass takes a `Matcher<CXXRecordDecl>, making it more user friendly.
aaron.ballman added inline comments.Jun 10 2020, 10:19 AM
clang/include/clang/ASTMatchers/ASTMatchers.h
2886–2888

Ah, might be good to put comments on there that say // doesn't match to make it more clear.

3541

FWIW, I prefer hasType to hasClass. You can inherit from things which are not a class, such as a struct (so the name is a bit of a misnomer, but not too awful), a class template (which you can't match with this interface), or a template type (which you also can't match with this interface).

@njames93 hasDirectBase seems like a useful matcher to me! OTOH I am not totally convinced about hasType -> hasClass. Anyway, don't you want to land hasDirectBase as a separate patch first and then discuss the rest?

One more thing - I'm just thinking if there isn't some corner case where a base class could be interpreted as both direct and indirect. The most ugly case I came up with is virtual inheritance. I admit I don't know what the standard says about this so it might be a non-issue. Also, it'd still probably make sense to match on such base. WDYT?

struct Base {};
struct Proxy : virtual Base {};
struct Derived : Base, Proxy {};
clang/include/clang/ASTMatchers/ASTMatchers.h
3541

I don't feel super strongly about this but I also slightly prefer hasType.

To be fair - I didn't really have things like inheritance from template parameters on my mind when working on hasAnyBase (it's definitely not tested) so I'd rather not assume it works.

3557

Nit: while "[base specifier] hasType" sounds natural to me for some reason hasClass doesn't. English is not my first language though.

clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
3168

Is this (Base != BAse) a typo or a way how to tease the asserts?

njames93 updated this revision to Diff 270000.Jun 10 2020, 4:43 PM
  • Added back hasType overload for CXXBaseSpecifier
  • Added hasClassTemplate and hasClassOrClassTemplate matcher for CXXBaseSpecifier
  • Added hasTemplatedDecl for ClassTemplateDecl
njames93 marked 2 inline comments as done.Jun 10 2020, 4:53 PM

@njames93 hasDirectBase seems like a useful matcher to me! OTOH I am not totally convinced about hasType -> hasClass. Anyway, don't you want to land hasDirectBase as a separate patch first and then discuss the rest?

There more I add to this, the more splitting it up sounds like a good idea.

One more thing - I'm just thinking if there isn't some corner case where a base class could be interpreted as both direct and indirect. The most ugly case I came up with is virtual inheritance. I admit I don't know what the standard says about this so it might be a non-issue. Also, it'd still probably make sense to match on such base. WDYT?

struct Base {};
struct Proxy : virtual Base {};
struct Derived : Base, Proxy {};

In that case it would probably register as a direct and indirect base. However there is no matcher(nor much need for one) that will solely match indirect bases so its mostly a non issue.

clang/include/clang/ASTMatchers/ASTMatchers.h
3541

I have decided to put hasType back in there as it does have some general uses. However I have added more class and class template specific matchers as I feel these are slightly more user friendly.

LMK what you think of this approach.

Side note what is the correct collective term for classes and structs. I'd be tempted to refer to them how clang does, records, but hasRecord seems wrong.

clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
3168

It was to tease the assers, but I removed it.

aaron.ballman added inline comments.Jun 11 2020, 11:17 AM
clang/include/clang/ASTMatchers/ASTMatchers.h
3541

Side note what is the correct collective term for classes and structs. I'd be tempted to refer to them how clang does, records, but hasRecord seems wrong.

We use the term "record", but I'm not certain how widely used that is.

3557

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")))))?

jkorous added inline comments.Jun 11 2020, 11:49 AM
clang/include/clang/ASTMatchers/ASTMatchers.h
3604

I think we should just use eachOf matcher for this kind of composition.

https://clang.llvm.org/docs/LibASTMatchersReference.html#traversal-matchers

jkorous added inline comments.Jun 11 2020, 11:52 AM
clang/include/clang/ASTMatchers/ASTMatchers.h
3557

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

njames93 marked 2 inline comments as done.Jun 11 2020, 3:59 PM
njames93 added inline comments.
clang/include/clang/ASTMatchers/ASTMatchers.h
3541

https://en.cppreference.com/w/cpp/language/class - Going of what that says, it states that a class declaration starts with a keyword either class or struct. Nowhere on the page does it mention record.
Continuing on from this point, we have many more matchers with class in the name but work on structs too:
ofClass, hasInClassInitializer and injectedClassNameType. If you're being pedantic there is also classTemplateDecl, classTemplatePartialSpecializationDecl and classTemplateSpecializationDecl.

Having said all of that I'm still not a huge fan of hasClass, but I'm less of a fan of hasType. I'd thought of forClass but that could be misinterpreted as the derived class of the CXXBaseSpecifier Kind of like the behaviour of forFunction.

class Base {};
class Derived : Base {};

Does cxxBaseSpecifier(forClass(cxxRecordDecl().bind("X")) bind to Derived` or Base?

3557

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

aaron.ballman added inline comments.Jun 12 2020, 6:07 AM
clang/include/clang/ASTMatchers/ASTMatchers.h
3557

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.

3604

+1

jkorous added inline comments.Jun 12 2020, 1:45 PM
clang/include/clang/ASTMatchers/ASTMatchers.h
3557

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.

What I meant is that technically we can overload any Matcher<CXXRecordDecl> matcher in the same fashion so having the overloaded version of hasName only makes it somewhat special (and someone might argue that it'd impact consistency of matchers composability). Anyway, I'm fine with your suggestion!

aaron.ballman added subscribers: sammccall, dblaikie.

Pinging @klimek , @sammccall , and @dblaikie to see if there are some opinions about overloading hasName (and possibly other naming related questions).

clang/include/clang/ASTMatchers/ASTMatchers.h
3557

Ah, I see what you mean -- thanks for explaining. I'm on the fence about this. One the one hand, base specifiers *in the AST* do not have names, so it seems defensible to say that hasName should not handle a base specifier. On the other hand, base specifiers *in the language* are identifiers that always have a name, so it seems defensible to say that hashName should handle a base specifier.

Pulling in some more folks to see if a wider audience brings consensus.

We have some precedent for overloading has* matchers, but I'll defer to Sam's judgement whether that's a good idea here.

sammccall added inline comments.Jun 15 2020, 8:25 AM
clang/include/clang/ASTMatchers/ASTMatchers.h
3557

I think I agree that it's reasonable/consistent to do this, but I'm not sure it's a good idea.
Base specifier -> type -> class -> name seems like the most *regular* traversal that reflects the shape/concepts of the AST, shortcutting steps makes the model (IMO) more complicated to make it more terse.
Is matching a base specifier with a hardcoded name really common enough in absolute terms that people should keep this special case in their heads?

I think hasType(cxxRecordDecl(hasName("Base"))) a bit better, but i think of *expressions* having types, and would prefer specifiesType or just specifies at the outside.
But just my 2c and I'm not deeply familiar with the conventions here - I can live with any of the options.

aaron.ballman added inline comments.Jun 16 2020, 5:29 AM
clang/include/clang/ASTMatchers/ASTMatchers.h
3557

Is matching a base specifier with a hardcoded name really common enough in absolute terms that people should keep this special case in their heads?

Thanks for the perspective. I don't think base specifiers will be matched all that often, but I'm also not certain it's a special case because base specifiers are conceptually things with names even if they're not exposed as such via our AST. However, you bring up a good point about modelling the shape of the AST and that does suggest base specifier -> type -> class -> name as the right way to go.

I think hasType(cxxRecordDecl(hasName("Base"))) a bit better, but i think of *expressions* having types, and would prefer specifiesType or just specifies at the outside.

If hasType only worked on expressions, I would be okay with adding specifies or specifiesType for this functionality, but since hasType works on expressions and declarations, I think it's more natural to reuse hasType for the specifier than to introduce specifiesType (or hasClass) for base specifiers (which name a class type).

njames93 updated this revision to Diff 275991.Jul 7 2020, 4:27 AM

Removed all hasClass related changes.

njames93 retitled this revision from [ASTMatchers] Added hasDirectBase and hasClass Matchers to [ASTMatchers] Added hasDirectBase Matcher.Jul 7 2020, 4:28 AM
njames93 edited the summary of this revision. (Show Details)

I've removed the hasClass changes as they dont entirely belong in here and weren't well received. Now with just the hasDirectBase this should be in a good state.

aaron.ballman accepted this revision.Jul 7 2020, 7:44 AM

LGTM with a whitespace nit.

clang/include/clang/ASTMatchers/ASTMatchers.h
2895

Can remove the newline.

This revision is now accepted and ready to land.Jul 7 2020, 7:44 AM
This revision was automatically updated to reflect the committed changes.