This looks good to me, but I think we need a deployment target check on the diagnostic since the safe API is only available in iOS 11+, macOS 10.13+, tvOS 11+, and watchOS 4.0+. If the deployment target is early than those versions then we should not diagnose.

Looks good do me. Thanks!

LGTM. Thanks!

Looks good to me.

Looks good to me!

Looks good to me. This is a really interesting corner of Objective-C++!

LGTM.

I'm not sure we really want to add any more examples of plugins.

This looks good to me. It is great to see this tested!

I think it would be better if the default for compatibility mode were 'true'. That way this change will be backwards compatible and clients who want to enforce stricter checking could enable it. Setting compatibility mode to be true in the driver is not sufficient since many build systems call the frontend directly.

I'm pretty worried about exposing this flag to end users.

Update to restrict the new behavior to when Xcode is present.

That's a good point. I've updated the patch to look for 'xcrun'

Sigh. Looks good!

LGTM.

Aah, MIG_NO_REPLY.

Oh, interesting. I'm glad you thought of this!

Looks good to me. I have some a minor diagnostic wording suggestion in line.

LGTM.

LGTM with a minor grammatical nits inline. It is great to see a checker for this!

This looks good to me as long as Aaron is happy with it.

Looks good to me.

This seems reasonable to me, but please have George take a look too.

Looks good to me.

These seems reasonable, although it does also seem like there could be quite a few unintended consequences that we haven't discovered yet.

LGTM.

This seems reasonable to me, although I have a question inline about why you are using makeZeroElementRegion().

This looks good to me. It is great to see a dumper for this!

Looks good to me!

Looks good! Some suggested minor tweaks to diagnostic text inline.

My mistake, sorry about that!

It is great to see a check for this!

It is really nice to see this checker take shape! Some drive by diagnostic comments in line.

This seems like a useful checker!

In D46159#1085548, @pfultz2 wrote:

Do you have some better choices?

I could do -allow-alpha-checks. What do you think?

In D46159#1081371, @lebedev.ri wrote:

Note that it is completely off by default, and is not listed in documentation, clang-tidy --help,
so one would have to know of the existence of that hidden flag first, and only then when that flag is passed, those alpha checks could be enabled.

I'm curious about the use case for this, since I don't think it ever makes sense to run all the alpha checks at once. These checks are typically a work in progress (they're not finished yet) and often have false positives that haven't been fixed yet. Often they don't handle all facets of the language and can crash on unhandled constructs. While it makes sense to enable a specific alpha check (or even a set of related alpha checks) when developing these checks or when evaluating whether they are ready to graduate into non-alpha it seems to me that running all of them isn't useful, ever.

Looks good with updated diagnostic text and a test for non-ARC behavior.

Looks good to me.

Looks good to me!

LGTM. I really appreciate the extra documentation you've added, too.

Looks good. It is nice to see this fixed. There is one testing nit inline

LGTM, but I do think Artem is right that we should push the user in the right direction here. There is a suggested addendum inline.

I put some post-commit comments in https://reviews.llvm.org/D44059 about diagnostic text and naming that you should address as part of graduating this out of alpha.

This looks good. Some minor post-commit review inline.

LGTM!

This looks good to me. I'm not super happy with the name "CFGValueTypedCall" since it doesn't make it obvious that is reflects "a function call that returns a C++ object by value."

Yay! This looks good to me.

Thanks Gabor, this looks good to me. Please commit!

LGTM. I have a suggestion for a slight modification of the diagnostic text inline.

LGTM.

In D43421#1018270, @george.karpenkov wrote:

Actually, after posting this I've realized that this behavior is somewhat inconsistent, and we should still produce an empty plist when requested to: just as we do for files with no generated reports.

Actually, after thinking about this some more, the answer is those plists should not be produced, as they are not produced in similar cases (disableAllChecks is set, etc).

In D43218#1018101, @george.karpenkov wrote:

...turns out the previous version had a bug. Re-introduced logging, as I've needed that for the second time for debugging.

This seems reasonable to me.

LGTM.

It seems kind of sketchy to me that we're recursing over an expression to find construction contexts and then later doing it again for sub-expressions. I guess there is precedent here with VisitForTemporaryDtors(), but we should think about whether there is a better way to do this.

Looks good to me. I have a comment about simplifying createTemporaryRegionIfNeeded() (if possible) inline.

This looks great to me Artem! I'll be very curious to see how you extend this to handle initializer lists.

We have a comment documenting the decision. Adding a printf doesn't seem like it would help very much here and adds untested code. I don't think we should have a policy of adding these printfs on every early return. Since these printfs make it harder to read the code and are duplicated with the comments, I think we should avoid adding one until we know it is actually generally useful.

We don't consider --analyze to be public UI, which is why it is not documented. The supported interface to the analyzer is scan-build.

This looks good to me, but I think you should include your explanatory comments in the commit message to the comment itself to help future violators of the assertion out!

This looks good to me, although as I noted inline I think both the name and the comment for VisitForConstructionContext() are confusing. If you can be more precise I think it would help future maintainers.