Reworked, per our discussion. PTAL.

Discarded API changes, reduced to new clang-tidy lib test.

Gentle ping...

In D93637#2479576, @tdl-g wrote:

Ah, if it's just an optimization that makes sense. I still think it's worth having a test case to confirm that one of the specially-handled cases works.

Added test for unique_ptr specifically.

Thanks for the review!

added comment known smart pointers.

thanks!

updated name, comments and tests.

renamed the combinator

In D91872#2408278, @aaron.ballman wrote:

Drive-by question from the peanut gallery, sorry if this is an ignorant one -- not all declarations have a trailing semicolon; is that handled properly? e.g., int x; has a trailing semicolon but int x, y; only has a trailing semicolon for one of the two declarations. Relatedly, in int f(int x);, the declaration of f has a trailing semicolon, but the declaration of x does not.

Clarified that semicolons are only removed if present. Fixed test.

LGTM. I'll leave it to Aaron, though, to accept.

cleaned up test code

TL;DR Stephen's fix works; I'll drop this patch.

In D88275#2316484, @aaron.ballman wrote:

Thank you for your patience while we sort this out. I've pinged @steveire off-list, so hopefully he'll respond with his thoughts. If we don't hear from Stephen in the next week or so, I think we should proceed with your proposed patch to get you unblocked (adding one more "ignore implicit" variant isn't the end of the world, I just want us to be thoughtful about whether we want to add new matchers in this space or to work on the traversal mode instead).

In D88275#2305989, @aaron.ballman wrote:

In D88275#2303283, @ymandel wrote:

I'm not concerned about the basic idea behind the proposed matcher, I'm only worried we're making AST matching more confusing by having two different ways of inconsistently accomplishing the same-ish thing.

Aaron, I appreciate this concern, but I would argue that this matcher isn't making things any worse. We already have the various ignoringImplicit matchers, and this new one simply parallels those, but for parents. So, it is in some sense "completing" an existing API, which together is an alternative to traverse.

I'm not certain I agree with that reasoning because you can extend it to literally any match that may interact with implicit nodes, which is the whole point to the spelled in source traversal mode. I'm not certain it's a good design for us to continue to add one-off ways to ignore implicit nodes.

I'm not concerned about the basic idea behind the proposed matcher, I'm only worried we're making AST matching more confusing by having two different ways of inconsistently accomplishing the same-ish thing.

Aaron, I appreciate this concern, but I would argue that this matcher isn't making things any worse. We already have the various ignoringImplicit matchers, and this new one simply parallels those, but for parents. So, it is in some sense "completing" an existing API, which together is an alternative to traverse.

restored changes to unrelated parts of html docs.

In D88275#2295379, @aaron.ballman wrote:

This seems to be strongly related to the TK_IgnoreUnlessSpelledInSource traversal behavior; is there a reason you can't use that traversal mode with hasParent() (does it behave differently)?

Fixed to use more standard type adaptors. Registration now works.

update dynamic registry and the ast matcher doc.

Thanks, this looks great! But, can you also please update https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/clang-tidy/modernize/UseEqualsDeleteCheck.cpp#L39, since it depends on the current semantics?

In D87527#2271059, @baloghadamsoftware wrote:

We must decide about the namings. If we want to be in sync with the methods in FunctionDecl, then we keep hasBody() as is, but remove the template specialization, and create a new doesThisDeclarationHaveABody(). However, this involves changing existing checks. The other solution is to fix hasBody() like in this patch, and find a new name for the other behavior, such as e.g. hasBodySomewhere() or something similar.

In D87527#2271016, @baloghadamsoftware wrote:

In D87527#2270939, @ymandel wrote:

Can you expand on what is wrong currently for FunctionDecl descendants? Would the new test FindsBodyOfFunctionChildren fail with the current implementation?

Yes, exactly. They return 2 findings instead of 1 because the matcher matches both the forward declaration and the definition. The cause for this is that template specializations do not work for the descendants of the class for which is template specialized. Instead, the generic version is used. Thus for the children of FunctionDecl the original GetBodyMatcher is instantiated which does not check whether doesThisDeclarationHaveABody() but returns a body instead which may be bound to another declaration of the same function in the declaration chain. This is obviously wrong.

Can you expand on what is wrong currently for FunctionDecl descendants? Would the new test FindsBodyOfFunctionChildren fail with the current implementation?

Thanks for the review!

fix diff base; make small clang-tidy suggested change

Moved new rewriteDescendants overloads to detail namespace

reword comment per suggestion.

fixed typo

fixed typo

Updated clang-tidy transformer interpreter correspondingly.

tweak

fix typo