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
rC Clang

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

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

clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
342

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
5297

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

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

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

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
2672

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).

2682

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

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