the test looks better now, another round of reviews, most are nits.

Verify the error message in the test.

Address review comments.

Address review comments.

More cleanups.

Address review comments.

Improve the tests.

Thanks, I think we can simplify the interface further, see my comments inline.

Use the hybrid approach based on our discussion, the implementation should be fine to review, I'm still working on adding more tests.

Remove the unrelated changes.

Switch to a hybrid solution (language semantic + index) to detect "symbol local".

Address review comments

In D63331#1548020, @nik wrote:

In D63331#1543441, @hokein wrote:

Yeah, we definitely have interest in this feature, and our intern @jvikstrom will work on this feature this summer.

"this summer" depends on the location, so can you concretize this? :)

In D63425#1546010, @kadircet wrote:

We also make use of PickOne in updatePreamble shouldn't that also cause similar troubles?

Address comments.

Yeah, we definitely have interest in this feature, and our intern @jvikstrom will work on this feature this summer.

In D63323#1543331, @kadircet wrote:

could you also bump the version numbers?

Address review comments.

looks good.

thanks, looks good to me. I'll commit for you. @gribozavr do you want to take another look on the patch?

looks most good to me, a few nits.

looks good.

Thanks for investigating these, nice work!

As discussed offline, we don't need a separate fix description for this check, as the check warning message clearly implies what the fix will do.

@juliehockett could you take a look on this patch? I think you have the best knowledge about this check and fuchsia stuff.

In D62845#1529190, @gribozavr wrote:

In D62845#1529149, @hokein wrote:

I think we should be looking at the intent of the test rather than its name.

The intent looks like testing how the check works when std::make_unique is available from the standard library (as opposed to some kind of replacement like absl::make_unique). See the patch that introduced it: https://reviews.llvm.org/D43766

So modernize-make-unique-cxx14 is actually "C++14 or later". (Probably it should be renamed.)

yeap, it seems to me that "modernize-make-unique-cxx14" is redundant, "modernize-make-unique" should cover what it tests, I believe. We also have "modernize-make-unique-cxx11" which runs on C++11 mode only, maybe we just repurpose the modernize-make-unique-cxx14, what do you think?

Fair enough.

I see. Assuming it is desired behavior, I'd say for these cases we should create separate files that are specifically run in C++14 and 17, and another one for C++2a onward.

But is it desired behavior? That is, can we generate a call to std::make_unique in C++14 in practice -- would it compile?

The fix is compilable for C++14, but it is tricky to support it:

1. new NoCopyMoveCtor{}: the make_unique fix is compilable
2. new NoCopyMoveCtor{1, 2}: the make_unique fix is not compilable
   
   The AST for case 1) and 2) are the same in C++14, supporting that would introduce hacky change to the logic here. I'd leave it as-is now.

Indeed, this is complicated. Could you add tests for new NoCopyMoveCtor{1, 2} with TODOs (the message suggests the user to do the impossible).

address review comments.

I think we should be looking at the intent of the test rather than its name.

The intent looks like testing how the check works when std::make_unique is available from the standard library (as opposed to some kind of replacement like absl::make_unique). See the patch that introduced it: https://reviews.llvm.org/D43766

So modernize-make-unique-cxx14 is actually "C++14 or later". (Probably it should be renamed.)

Tests for C++2a.

In D62845#1528791, @gribozavr wrote:

I'd suggest to add a separate file that covers the exact language modes needed.

The C++14 test that we have right now is about C++14-or-later, testing the availability of std::make_unique.

nice, looks good