LGTM aside from some minor nits.

LGTM aside from a nit and a test case request.

In terms of implementation, the attribute code looks pretty close (just a few nits). However, someone more familiar with Obj-C should chime in with whether the attribute semantics make sense and whether the documentation will be sufficiently clear for an Objective-C user.

LGTM aside from some nit cleanup.

In D55394#1356941, @rjmccall wrote:

This is the AST dumper. This is not a user feature.

LGTM!

LGTM aside from a nit.

LGTM!

LGTM aside from auto nits.

In D55394#1356837, @rjmccall wrote:

I don't really have an opinion about this, sorry.

It's still missing the tests in SemaObjC -- you can use clang\test\SemaObjC\attr-root-class.m as an example of what I'm talking about.

Adding @rjmccall as an Obj-C expert to see if he has opinions on the output changes. Also, pinging @rsmith in case he'd like to weigh in.

Mostly looks good (a few minor nits), but I did have a design question about whether we should prefer calling a Visit method for type hierarchies instead of reimplementing the functionality.

LGTM aside from some minor nits.

LGTM!

LGTM!

LGTM aside from some minor nits.

Aside from nits that aren't complete yet, LGTM.

LGTM!

The patch is missing SemaObjC tests that ensure the attribute only appertains to the expected subjects, accepts no args, etc.

LGTM aside from some minor nits.

LGTM!

I've commit in r350891 (clang part) and r350892 (clang-tools-extra part). If @rsmith has concerns, we can fix or revert when raised.

LGTM!

LGTM with one minor testing request (conditional on the test passing, of course).

In general, I agree that it doesn't make sense for ODR violations (with false positives) to trigger error diagnostics while performing AST merging, so downgrading this to warnings is a good idea. However, I do worry about impacting modules and C compatibility with the changes. I'm curious what @rsmith thinks the answer should be here. AFAIK, modules do not *require* ODR violations checks (I believe that's still ill-formed, no diagnostic required), but confirmation would be good.

LGTM!

Aside from cosmetic nits, this LGTM.

LGTM!

Can you add a test that checks that a lambda declared within another lambda also traverses as expected?

@rsmith -- do you have opinions on the new format for how we display the child node? I think this is a more clear approach, but a second opinion would be nice.

In D56444#1351127, @steveire wrote:

I suggest not trying to make any such drastic changes for 8.0, try to fix the bug in a minimal way if possible, and have a more considered approach to the future of AST Matchers for after the release.

In D56444#1351096, @sammccall wrote:

In D56444#1351056, @aaron.ballman wrote:

Given that, I kind of think we should have functionDecl() match only functions, and give users some other way to match the semantic declarations in a consistent manner. Alternatively, we could decide semantics are what we want to match (because it's what the AST encodes) and instead we give users a way to request to only match syntax.

I believe matching the implied semantic nodes is how closer to how matchers behave in general (corresponding to the fact that the ASTMatcher RecursiveASTVisitor sets shouldVisitImplicitCode to true). e.g.

In D56444#1351067, @klimek wrote:

In D56444#1351056, @aaron.ballman wrote:

In D56444#1350849, @JonasToth wrote:

In D56444#1350746, @sammccall wrote:

@klimek: would it be better to preserve the odd behavior of the functionDecl() matcher, and add a new functionOrLambdaDecl()? It seems too surprising to me.

We can change the clang-tidy check as well. I think lambdas should be considered functionDecls, shouldnt they? WDYT @aaron.ballman ?

I think it depends on whether we want the AST matchers to match what the user wrote (syntax) or how the program behaves (semantics). If the user writes a lambda, they did not syntactically write a function declaration and so it makes sense for functionDecl() to not match. However, the semantics of a lambda are that they act as a functor (in effect) which includes a class declaration, a function declaration for the function call operator, conversion operators, etc and so it does make sense for users to want to match those semantic components. Given that, I kind of think we should have functionDecl() match only functions, and give users some other way to match the semantic declarations in a consistent manner. Alternatively, we could decide semantics are what we want to match (because it's what the AST encodes) and instead we give users a way to request to only match syntax.

The problem is that while I agree it would be nice if we matched either exactly the semantic or syntactic form of the code, the reality is that we match whatever the AST represents when we visit all nodes. Instead o f promising folks something that we can't hold (because nobody has time to fully check which matchers will match which form), I think telling people that we match the AST is the more honest and less surprising result in the end.

In D56444#1350849, @JonasToth wrote:

In D56444#1350746, @sammccall wrote:

@klimek: would it be better to preserve the odd behavior of the functionDecl() matcher, and add a new functionOrLambdaDecl()? It seems too surprising to me.

We can change the clang-tidy check as well. I think lambdas should be considered functionDecls, shouldnt they? WDYT @aaron.ballman ?

Once D56407 lands, the rebased changes here LGTM

LGTM aside from the nits pointed out; you can fix and commit without another round of review.

I think this is a good approach to solving the problem, so let's go this route.

LGTM!

LGTM aside from a formatting nit.

LGTM!

In D55212#1347994, @arichardson wrote:

I don't see an easy way of fixing the pragma clang attribute support for this.
Would it be okay to commit this change anyway?

The downside to this change is that callers now have to check the return values for validity where they didn't previously because the assertion covered it. However, I think that's probably reasonable in these cases since they're mostly returning source locations or ranges.

In D56301#1346425, @ZaMaZaN4iK wrote:

@hokein Is there any policy about the question "Which changes can clang-tidy check suggest?" . Should clang-tidy check suggest only "safe" fixes and warn only in "safe" cases?

LGTM! Thanks, both!

LGTM aside from a few minor nits/questions.

I commit this in r350414, thank you for the patch!

Committed with minor modifications for the range-based for loop changes in r350404.

LGTM!

Adding Manuel for the hasArg() questions.

Committed in r350317.

Ping

Updated based on review feedback. The vast majority of the changes are from the removal of a default argument to ActOnFullExpr().

This looks great to me (aside from a minor commenting nit). Thank you for the fix!

In D56090#1341023, @hwright wrote:

@lebedev.ri Where do the appropriate tests live? (I couldn't find an obvious subdirectory in test/)

The docs are getting much closer -- just a few grammatical things left to fix, I believe.

Updated based on review feedback.

Adding @tonic as a reviewer, who may have suggestions on how to improve the documentation wording.

LGTM!

There's enough churn based on the review feedback that this should probably have a second round of review just to be sure.