Page MenuHomePhabricator

[ASTMatchers] Matchers related to C++ inheritance
ClosedPublic

Authored by jkorous on Apr 28 2020, 8:14 PM.

Diff Detail

Event Timeline

jkorous created this revision.Apr 28 2020, 8:14 PM

Can you run clang-format over this patch please.
Personally I'm also in favour of removing the Base from the access modifiers i.e. isPublicBase to isPublic.
As you are adding matchers you need to do a few small other jobs:

  • Add these matchers to clang/lib/ASTMatchers/Dynamic/Registry.cpp. Make sure you keep it in alphabetical order.
  • Regenerate the docs by running clang/docs/tools/dump_ast_matchers.py.
clang/include/clang/ASTMatchers/ASTMatchers.h
2899

s/public/private

2907

NIT: remove the empty comment line, same goes below.

clang/lib/ASTMatchers/ASTMatchFinder.cpp
947 ↗(On Diff #260826)

Why are you casting away const, setOrigin takes a const CXXRecordDecl *? Think of the kittens life :)

jkorous marked 4 inline comments as done.May 12 2020, 5:22 PM
jkorous added inline comments.
clang/lib/ASTMatchers/ASTMatchFinder.cpp
947 ↗(On Diff #260826)

Haha :D
Thanks a bunch! I totally forgot to remove this after I landed the patch tweaking origin in CXXBasePaths.

jkorous updated this revision to Diff 263580.May 12 2020, 5:25 PM
jkorous marked an inline comment as done.
jkorous edited the summary of this revision. (Show Details)
  • Changed matchers to polymorphic instead of using Base suffix.
  • Registered one new matcher (the rest has been already existing and is now just polymorphic).
  • Regenerated the docs.
  • Some doc comments tweaking.
  • Saved a kitten!

A few more nits, you'll need to re-run the python script after addressing them.

clang/include/clang/ASTMatchers/ASTMatchers.h
561

I'm struggling to see the value of this matcher. I don't think its possible to use it as a match query e.g.
match cxxBaseSpecifier() The only possible use case I can think of is maybe to try to bind to it, though I'm not even sure that would work.

2882–2891

Gonna hazard a guess you forgot to remove this.

3522

Could you update the comment to say Usable as ... Matcher<CXXBaseSpecifier> and maybe a use case in the example.

5241–5252

Again add a use case in the example.

jkorous updated this revision to Diff 264752.May 18 2020, 4:25 PM
jkorous marked 4 inline comments as done.
  • addressed the comments
  • regenerated docs
njames93 added inline comments.May 19 2020, 2:59 AM
clang/lib/ASTMatchers/ASTMatchersInternal.cpp
727

As you have removed the decl in ASTMatchers.h this no longer needs to be here

jkorous marked 2 inline comments as done.May 19 2020, 1:53 PM
jkorous added inline comments.
clang/lib/ASTMatchers/ASTMatchersInternal.cpp
727

True!

jkorous updated this revision to Diff 265029.May 19 2020, 1:55 PM
jkorous marked an inline comment as done.

You really don't need to modify the MatchASTVisitor for this.
I just put this in ASTMatchersInternal.cpp with a decl in ASTMatchersInternal.h

bool matchesAnyBase(const CXXRecordDecl &Node,
                    const Matcher<CXXBaseSpecifier> &BaseSpecMatcher,
                    ASTMatchFinder *Finder, BoundNodesTreeBuilder *Builder) {
  if (!Node.hasDefinition())
    return false;

  CXXBasePaths Paths;
  Paths.setOrigin(&Node);

  const auto basePredicate =
      [Finder, Builder, &BaseSpecMatcher](const CXXBaseSpecifier *BaseSpec,
                                            CXXBasePath &IgnoredParam) {
        BoundNodesTreeBuilder Result(*Builder);
        if (BaseSpecMatcher.matches(*BaseSpec, Finder, Builder)) {
          *Builder = std::move(Result);
          return true;
        }
        return false;
      };

  return Node.lookupInBases(basePredicate, Paths,
                            /*LookupInDependent =*/true);
}

Then your matcher code goes to

AST_MATCHER_P(CXXRecordDecl, hasAnyBase, internal::Matcher<CXXBaseSpecifier>,
              BaseSpecMatcher) {
  return internal::matchesAnyBase(Node, BaseSpecMatcher, Finder, Builder);
}

Theres also a file clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.cpp that has a compile error due to the changes in isProtected and isPrivate that needs fixing.

You really don't need to modify the MatchASTVisitor for this.

Perfect! Thanks for figuring this out!

jkorous updated this revision to Diff 265324.May 20 2020, 12:25 PM
jkorous edited the summary of this revision. (Show Details)
  • Avoid changing the MatchASTVisitor.
  • Fix clang-tidy build failure.

LGTM but I'm not the gatekeeper, see what Manuel says first.

clang/lib/ASTMatchers/ASTMatchFinder.cpp
21 ↗(On Diff #265324)

Nit, this can be removed, its the only change to this file

jkorous updated this revision to Diff 265545.May 21 2020, 10:59 AM
jkorous marked an inline comment as done.

Don't add header that is now unnecessary

I'd like to land this. @klimek do you have any thoughts?

nick added a subscriber: nick.May 26 2020, 5:09 PM
nick added inline comments.
clang/include/clang/ASTMatchers/ASTMatchers.h
561

Actually, I was needed exactly that for D69000 and proposed a patch in D69218, though it has not received much attention.

I'm adding couple other potential reviewers. @njames93 if you feel comfortable doing that I think you can approve yourself too.

aaron.ballman accepted this revision.May 29 2020, 4:26 AM

LGTM aside from a nit.

clang/include/clang/ASTMatchers/ASTMatchers.h
2867

The example doesn't include hasAnyBase?

This revision is now accepted and ready to land.May 29 2020, 4:26 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 29 2020, 1:07 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript