This is an archive of the discontinued LLVM Phabricator instance.

[clang] Add isDirectlyDerivedFrom AST Matcher.
ClosedPublic

Authored by AntonBikineev on Jul 22 2019, 7:10 AM.

Details

Summary

This patch adds isDirectlyDerivedFrom AST-matcher which is similar to isDerivedFrom but only matches against direct base classes.

Diff Detail

Repository
rL LLVM

Event Timeline

AntonBikineev created this revision.Jul 22 2019, 7:10 AM
klimek added inline comments.Jul 22 2019, 10:39 AM
clang/include/clang/ASTMatchers/ASTMatchers.h
2637 ↗(On Diff #211091)

/*Directly=*/false
(and similarly below for true)

clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
342 ↗(On Diff #211091)

I think these tests are ~enough, given the implementation.
Specifically, I think duplicating all the negative tests is not covering more ground.
If you disagree, feel free to argue back :)

Manuel, I left some cases which deviated from the isDerivedFrom matcher.

klimek requested changes to this revision.Jul 22 2019, 12:24 PM
klimek added inline comments.
clang/docs/LibASTMatchersReference.html
5277 ↗(On Diff #211148)

Did you manually change this? This should be generated from the code, but I don't see this text anywhere in the code comments?

This revision now requires changes to proceed.Jul 22 2019, 12:24 PM

Thanks for the note, updated the diff with autogenerated LibASTMatcherReference.html.

Manuel, would you mind taking another look at the change please?

So, I did like the more exhaustive doc, I thought you'd move it to the code so it also shows up in the generated doc page :) (sorry for not being clearer here)

clang/include/clang/ASTMatchers/ASTMatchers.h
2637 ↗(On Diff #211261)

If you say /*Directly=*/false, you should
a) not get a space from clang-format
b) clang-tidy can check it's the right parameter name :)

aaron.ballman added inline comments.
clang/include/clang/ASTMatchers/ASTMatchers.h
2672 ↗(On Diff #211261)

I don't think this assertion is reasonable -- we should instead test this as a predicate and return false if the base name is empty.

AntonBikineev marked an inline comment as done.

Thanks for the comments!

clang/include/clang/ASTMatchers/ASTMatchers.h
2672 ↗(On Diff #211261)

It's done in the same way as for existing isDerivedFrom and isSameOrDerivedFrom matchers. Maybe it would make sense to change all of them, but I guess it should rather be a separate commit.

aaron.ballman added inline comments.Jul 24 2019, 8:04 AM
clang/include/clang/ASTMatchers/ASTMatchers.h
2682 ↗(On Diff #211508)

You should register this in Registry.cpp as an overload, like we do for isDerivedFrom() and isSameOrDerivedFrom().

2672 ↗(On Diff #211261)

I'm fine with doing it in a follow-up commit, but it should be done (it shouldn't assert on invalid input, only on thought-to-be impossible situations, generally speaking).

AntonBikineev marked an inline comment as not done.

Missed that, thanks for the point!

This revision was not accepted when it landed; it landed in state Needs Review.Jul 25 2019, 4:53 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 25 2019, 4:53 AM