This is an archive of the discontinued LLVM Phabricator instance.

[ASTMatchers] new forEachOverriden matcher
ClosedPublic

Authored by courbet on Apr 20 2016, 6:52 AM.

Details

Summary

Matches methods overridden by the given method.

Diff Detail

Event Timeline

courbet updated this revision to Diff 54363.Apr 20 2016, 6:52 AM
courbet retitled this revision from to [ASTMatchers] new forEachOverriden matcher.
courbet updated this object.
courbet added a subscriber: cfe-commits.
mgrang added a subscriber: mgrang.Apr 20 2016, 11:16 AM
mgrang added inline comments.
include/clang/ASTMatchers/ASTMatchers.h
3719

Extraneous ", and" at the end of comment.

courbet updated this revision to Diff 54462.Apr 20 2016, 11:50 PM
courbet marked an inline comment as done.

Fix typo in doc.

aaron.ballman added a subscriber: aaron.ballman.

Please run clang\docs\tools\dump_ast_matchers.py to regenerate the documentation as well.

include/clang/ASTMatchers/ASTMatchers.h
3724

Can you range-ify this for loop?

unittests/ASTMatchers/ASTMatchersTest.cpp
2084

Can you write the tests such that they don't leak?

courbet added inline comments.Apr 21 2016, 5:55 AM
unittests/ASTMatchers/ASTMatchersTest.cpp
2084

matchAndVerifyResultConditionally deletes them:

template <typename T>
testing::AssertionResult
matchAndVerifyResultConditionally(const std::string &Code, const T &AMatcher,
                                  BoundNodesCallback *FindResultVerifier,
                                  bool ExpectResult) {
  std::unique_ptr<BoundNodesCallback> ScopedVerifier(FindResultVerifier);

I'll send you a (separate) patch for taking a unique_ptr in.

courbet added inline comments.Apr 21 2016, 6:09 AM
include/clang/ASTMatchers/ASTMatchers.h
3724

I could do that , but it would require adding:

const CXXMethodVector* ASTContext::overridden_methods() const;

And plumbing it through. (or did I miss something ?)

Let me know what you think.

aaron.ballman added inline comments.Apr 21 2016, 6:14 AM
include/clang/ASTMatchers/ASTMatchers.h
3724

I think it's an oversight that we don't have the range form of that function, and this seems like a reasonable use case to add it.

courbet updated this revision to Diff 54512.Apr 21 2016, 8:33 AM
courbet marked 4 inline comments as done.
  • add overridden_methods() to CXXMethodDecl and plumbing.
  • Use range-based for loop for iterating over overridden methods.
courbet marked an inline comment as done.Apr 21 2016, 8:34 AM
courbet updated this revision to Diff 54514.Apr 21 2016, 8:49 AM

Regenerate doc.

sbenza added inline comments.Apr 21 2016, 10:31 AM
lib/AST/ASTContext.cpp
1279 ↗(On Diff #54514)

It would be simpler to return ArrayRef<const CXXMethodDecl*>.
That way the caller doesn't need to deal with a potential null pointer or with the custom vector type.

courbet updated this revision to Diff 54830.Apr 25 2016, 4:26 AM
courbet marked an inline comment as done.

overridden_methods return ArrayRef instead of TinyPtrVector*

sbenza added inline comments.Apr 26 2016, 9:57 AM
include/clang/AST/DeclCXX.h
1830 ↗(On Diff #54830)

Return type should have have toplevel const.

lib/AST/ASTContext.cpp
1262 ↗(On Diff #54830)

I would invert the calls here.
That is, make overridden_methods_begin/_end call overridden_methods() instead, and have overridden_methods() be the one that does the lookup.
This way we have a single place where the lookup happens. It would also make overridden_methods() faster.

courbet updated this revision to Diff 56006.May 3 2016, 8:29 AM
courbet marked an inline comment as done.

implement overridden_methods_begin()/end() in terms of overridden_methods().

aaron.ballman added inline comments.May 3 2016, 8:41 AM
include/clang/AST/ASTContext.h
824 ↗(On Diff #56006)

This is too tight of a coupling to the underlying datatype. It should return iterator_range<overridden_cxx_method_iterator>

include/clang/AST/DeclCXX.h
1830 ↗(On Diff #56006)

This should return iterator_range<method_iterator>

courbet added inline comments.May 3 2016, 8:59 AM
include/clang/AST/ASTContext.h
824 ↗(On Diff #56006)

This is the exact opposite of the change that Samuel just requested (implementing the iterators in term of the ArrayRef getter). I don't have a strong opinion on this, but could you two agree on the desired API ?

aaron.ballman added inline comments.May 3 2016, 9:09 AM
include/clang/AST/ASTContext.h
824 ↗(On Diff #56006)

I hadn't noticed that we disagreed, sorry for the conflicting advice. However, I strongly think that we should use an abstraction, and that ArrayRef is too concrete. FWIW, we use iterator_range in almost every other case in the code base when we refactored to rangify code.

courbet added inline comments.May 3 2016, 9:11 AM
include/clang/AST/ASTContext.h
824 ↗(On Diff #56006)

Samuel, are you OK with this (reverting the last change and using iterator_range instead of ArrayRef) ?

courbet updated this revision to Diff 57462.May 17 2016, 5:00 AM

Use iterator_range instead of ArrayRef for overridden_methods

aaron.ballman accepted this revision.May 17 2016, 5:58 AM
aaron.ballman edited edge metadata.

LGTM, with one minor nit. Thank you for working on this!

include/clang/ASTMatchers/ASTMatchers.h
3724

Nit: the * should bind to Overridden instead of auto. (May just want to clang-format the patch.)

This revision is now accepted and ready to land.May 17 2016, 5:58 AM
courbet updated this revision to Diff 57473.May 17 2016, 6:49 AM
courbet edited edge metadata.

cometics

Thanks. Could please you submit this for me ?

include/clang/ASTMatchers/ASTMatchers.h
3724

Thanks for the catch. Unfortunately there are a lot of errors in the file that prevent me to run clang-format. Any hint to handle that efficiently ?

aaron.ballman added inline comments.May 18 2016, 5:18 AM
include/clang/ASTMatchers/ASTMatchers.h
3724

There's a script that you can use to reformat just the contents of a diff: http://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting

courbet updated this revision to Diff 57756.EditedMay 19 2016, 2:56 AM

Rebase on origin/master.

courbet updated this revision to Diff 57757.May 19 2016, 3:01 AM

clang-format diff

include/clang/ASTMatchers/ASTMatchers.h
3724

Thanks for the pointer !

courbet marked 10 inline comments as done.May 20 2016, 5:19 AM
sbenza added inline comments.May 23 2016, 8:30 AM
include/clang/AST/ASTContext.h
824 ↗(On Diff #57757)

Sure. Sorry about that.
The main idea was to use a range-like API instead of returning a nullable pointer to a range.

courbet updated this revision to Diff 59502.Jun 3 2016, 2:00 AM

Rebase on origin/master

@sbenza @aaron.ballman I don't have write access. Can one of you submit this on my behalf ? Thanks !

This comment was removed by courbet.
courbet closed this revision.Jun 10 2016, 5:06 AM

This was submitted as r272386.