[clang-tidy] Fix make-unique tests on C++2a.

[hokein]

Summary

These test cases are illgal in C++2a ("new Foo{}" needs to see the
default constructor), so move them to the C++14-only tests.

[gribozavr]

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.

[gribozavr]

I'm also not sure what the intent behind these tests is. Maybe the right fix is to add a constructor that can be called?

[hokein]

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.

The test file name ("modernize-make-unique-cxx14") indicates this test is for C++14, and since we change the existing modernize-make-unique test (which covers more cases) to C++14 or later, I think it is reasonable to restrict the cxx14 test to run only on C++14. or am I missing anything?

I'm also not sure what the intent behind these tests is. Maybe the right fix is to add a constructor that can be called?

sorry, the check is too complicated to catch up, the test cases are testing the code path https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/clang-tidy/modernize/MakeSmartPtrCheck.cpp#L405.

Unfortunately, adding a default constructor doesn't fix the problem: the AST tree is different in C++14/17 and C++2a, which causes different behavior in the check.

e.g. new NoCopyMoveCtor{}

In C++14/17, it looks like below, the check thinks it is an aggregate initialization (with deleted copy/move constructor) and doesn't generate the fix.

|-CXXNewExpr <line:26:3, col:22> 'NoCopyMoveCtor *' Function 0x5614cfece8f0 'operator new' 'void *(std::size_t)'
    | `-InitListExpr <col:21, col:22> 'NoCopyMoveCtor'

However, in C++2a, the AST is like below, the check thinks it is a direct initialization with default constructor, and generate the fix.

`-CXXNewExpr <line:26:3, col:22> 'NoCopyMoveCtor *' Function 0x55c9b1b24ba0 'operator new' 'void *(std::size_t)'
      `-CXXConstructExpr <col:7, col:22> 'NoCopyMoveCtor' 'void () noexcept' list zeroing

[gribozavr]

In D62845#1528887, @hokein wrote:

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.

The test file name ("modernize-make-unique-cxx14") indicates this test is for C++14, and since we change the existing modernize-make-unique test (which covers more cases) to C++14 or later, I think it is reasonable to restrict the cxx14 test to run only on C++14. or am I missing anything?

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.)

I'm also not sure what the intent behind these tests is. Maybe the right fix is to add a constructor that can be called?

sorry, the check is too complicated to catch up, the test cases are testing the code path https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/clang-tidy/modernize/MakeSmartPtrCheck.cpp#L405.

Unfortunately, adding a default constructor doesn't fix the problem: the AST tree is different in C++14/17 and C++2a, which causes different behavior in the check.

e.g. new NoCopyMoveCtor{}

In C++14/17, it looks like below, the check thinks it is an aggregate initialization (with deleted copy/move constructor) and doesn't generate the fix.

|-CXXNewExpr <line:26:3, col:22> 'NoCopyMoveCtor *' Function 0x5614cfece8f0 'operator new' 'void *(std::size_t)'
    | `-InitListExpr <col:21, col:22> 'NoCopyMoveCtor'

However, in C++2a, the AST is like below, the check thinks it is a direct initialization with default constructor, and generate the fix.

`-CXXNewExpr <line:26:3, col:22> 'NoCopyMoveCtor *' Function 0x55c9b1b24ba0 'operator new' 'void *(std::size_t)'
      `-CXXConstructExpr <col:7, col:22> 'NoCopyMoveCtor' 'void () noexcept' list zeroing

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?

[hokein]

Tests for C++2a.

[hokein]

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?

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.

[gribozavr]

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).

clang-tools-extra/test/clang-tidy/modernize-make-unique-cxx14.cpp

1

WDYT about merging two tests and adding run lines like this:

// RUN: %check_clang_tidy -std=c++14,c++17 -check-suffix=CXX_14_17 %s modernize-make-unique %t -- -- -I %S/Inputs/modernize-smart-ptr -D CXX_14_17=1
// RUN: %check_clang_tidy -std=c++2a -check-suffix=CXX_2A %s modernize-make-unique %t -- -- -I %S/Inputs/modernize-smart-ptr -DCXX_2A=1

[hokein]

address review comments.

[hokein]

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).

Done, but note that adding extra data fields to the NoCopyMoveCtor is uncompilable in C++2a :(

clang-tools-extra/test/clang-tidy/modernize-make-unique-cxx14.cpp

1

Good point, done!

[gribozavr]

Looks great, thank you!

clang-tools-extra/test/clang-tidy/modernize-make-unique-cxx14.cpp

-// RUN: %check_clang_tidy -std=c++14-or-later %s modernize-make-unique %t -- -- -I %S/Inputs/modernize-smart-ptr
+// RUN: %check_clang_tidy -std=c++14 %s modernize-make-unique %t -- -- -I %S/Inputs/modernize-smart-ptr
 
 #include "unique_ptr.h"
 // CHECK-FIXES: #include <memory>
 
+struct NoCopyMoveCtor {
+  NoCopyMoveCtor(const NoCopyMoveCtor &) = delete; // implies move ctor is deleted
+};
+
+struct NoCopyMoveCtorVisible {
+private:
+  NoCopyMoveCtorVisible(const NoCopyMoveCtorVisible&) = default;
+  NoCopyMoveCtorVisible(NoCopyMoveCtorVisible&&) = default;
+};
+
+struct OnlyMoveCtor {
+  OnlyMoveCtor() = default;
+  OnlyMoveCtor(OnlyMoveCtor&&) = default;
+  OnlyMoveCtor(const OnlyMoveCtor &) = delete;
+};
+
+struct OnlyCopyCtor {
+  OnlyCopyCtor(const OnlyCopyCtor&) = default;
+  OnlyCopyCtor(OnlyCopyCtor&&) = delete;
+};
+
+struct OnlyCopyCtorVisible {
+  OnlyCopyCtorVisible(const OnlyCopyCtorVisible &) = default;
+
+private:
+  OnlyCopyCtorVisible(OnlyCopyCtorVisible &&) = default;
+};
+
+struct ImplicitDeletedCopyCtor {
+  const OnlyMoveCtor ctor;
+};
+
 void f() {
   auto my_ptr = std::unique_ptr<int>(new int(1));
   // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: use std::make_unique instead
   // CHECK-FIXES: auto my_ptr = std::make_unique<int>(1);
+
+  // No fixes for classes with deleted copy&move constructors.
+  auto PNoCopyMoveCtor = std::unique_ptr<NoCopyMoveCtor>(new NoCopyMoveCtor{});
+  // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: use std::make_unique instead
+  // CHECK-FIXES: auto PNoCopyMoveCtor = std::unique_ptr<NoCopyMoveCtor>(new NoCopyMoveCtor{});
+
+  auto PNoCopyMoveCtorVisible = std::unique_ptr<NoCopyMoveCtorVisible>(new NoCopyMoveCtorVisible{});
+  // CHECK-MESSAGES: :[[@LINE-1]]:33: warning: use std::make_unique instead
+  // CHECK-FIXES: auto PNoCopyMoveCtorVisible = std::unique_ptr<NoCopyMoveCtorVisible>(new NoCopyMoveCtorVisible{});
+
+  auto POnlyMoveCtor = std::unique_ptr<OnlyMoveCtor>(new OnlyMoveCtor{});
+  // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: use std::make_unique instead
+  // CHECK-FIXES: auto POnlyMoveCtor = std::unique_ptr<OnlyMoveCtor>(new OnlyMoveCtor{});
+
+  auto POnlyCopyCtor = std::unique_ptr<OnlyCopyCtor>(new OnlyCopyCtor{});
+  // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: use std::make_unique instead
+  // CHECK-FIXES: auto POnlyCopyCtor = std::unique_ptr<OnlyCopyCtor>(new OnlyCopyCtor{});
+
+  auto POnlyCopyCtorVisible = std::unique_ptr<OnlyCopyCtorVisible>(new OnlyCopyCtorVisible{});
+  // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: use std::make_unique instead
+  // CHECK-FIXES: auto POnlyCopyCtorVisible = std::unique_ptr<OnlyCopyCtorVisible>(new OnlyCopyCtorVisible{});
+
+  auto PImplicitDeletedCopyCtor = std::unique_ptr<ImplicitDeletedCopyCtor>(new ImplicitDeletedCopyCtor{});
+  // CHECK-MESSAGES: :[[@LINE-1]]:35: warning: use std::make_unique instead
+  // CHECK-FIXES: auto PImplicitDeletedCopyCtor = std::unique_ptr<ImplicitDeletedCopyCtor>(new ImplicitDeletedCopyCtor{});
 }

clang-tools-extra/test/clang-tidy/modernize-make-unique.cpp

-// RUN: %check_clang_tidy -std=c++14,c++17 %s modernize-make-unique %t -- -- -I %S/Inputs/modernize-smart-ptr
-// FIXME: Fix the test code in C++2a mode.
+// RUN: %check_clang_tidy -std=c++14-or-later %s modernize-make-unique %t -- -- -I %S/Inputs/modernize-smart-ptr
 
 #include "unique_ptr.h"
 #include "initializer_list.h"
 // CHECK-FIXES: #include <memory>
 
 struct Base {
   Base();
   Base(int, int);

 
 template<typename T>
 struct MyVector {
   MyVector(std::initializer_list<T>);
 };
 
 struct Empty {};
 
-struct NoCopyMoveCtor {
-  NoCopyMoveCtor(const NoCopyMoveCtor &) = delete; // implies move ctor is deleted
-};
-
-struct NoCopyMoveCtorVisible {
-private:
-  NoCopyMoveCtorVisible(const NoCopyMoveCtorVisible&) = default;
-  NoCopyMoveCtorVisible(NoCopyMoveCtorVisible&&) = default;
-};
-
-struct OnlyMoveCtor {
-  OnlyMoveCtor() = default;
-  OnlyMoveCtor(OnlyMoveCtor&&) = default;
-  OnlyMoveCtor(const OnlyMoveCtor &) = delete;
-};
-
-struct OnlyCopyCtor {
-  OnlyCopyCtor(const OnlyCopyCtor&) = default;
-  OnlyCopyCtor(OnlyCopyCtor&&) = delete;
-};
-
-struct OnlyCopyCtorVisible {
-  OnlyCopyCtorVisible(const OnlyCopyCtorVisible &) = default;
-
-private:
-  OnlyCopyCtorVisible(OnlyCopyCtorVisible &&) = default;
-};
-
-struct ImplicitDeletedCopyCtor {
-  const OnlyMoveCtor ctor;
-};
 
 struct E {
   E(std::initializer_list<int>);
   E();
 };
 
 struct F {
   F(std::initializer_list<int>);
void initialization(int T, Base b) {
   // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: use std::make_unique instead
   // CHECK-FIXES: std::unique_ptr<DPair> PDir6 = std::make_unique<DPair>();
 
   // Aggregate initialization without arguments.
   std::unique_ptr<Empty> PEmpty = std::unique_ptr<Empty>(new Empty{});
   // CHECK-MESSAGES: :[[@LINE-1]]:35: warning: use std::make_unique instead
   // CHECK-FIXES: std::unique_ptr<Empty> PEmpty = std::make_unique<Empty>(Empty{});
 
-  // No fixes for classes with deleted copy&move constructors.
-  auto PNoCopyMoveCtor = std::unique_ptr<NoCopyMoveCtor>(new NoCopyMoveCtor{});
-  // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: use std::make_unique instead
-  // CHECK-FIXES: auto PNoCopyMoveCtor = std::unique_ptr<NoCopyMoveCtor>(new NoCopyMoveCtor{});
-
-  auto PNoCopyMoveCtorVisible = std::unique_ptr<NoCopyMoveCtorVisible>(new NoCopyMoveCtorVisible{});
-  // CHECK-MESSAGES: :[[@LINE-1]]:33: warning: use std::make_unique instead
-  // CHECK-FIXES: auto PNoCopyMoveCtorVisible = std::unique_ptr<NoCopyMoveCtorVisible>(new NoCopyMoveCtorVisible{});
-
-  auto POnlyMoveCtor = std::unique_ptr<OnlyMoveCtor>(new OnlyMoveCtor{});
-  // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: use std::make_unique instead
-  // CHECK-FIXES: auto POnlyMoveCtor = std::unique_ptr<OnlyMoveCtor>(new OnlyMoveCtor{});
-
-  // Fix for classes with classes with move constructor.
-  auto POnlyCopyCtor = std::unique_ptr<OnlyCopyCtor>(new OnlyCopyCtor{});
-  // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: use std::make_unique instead
-  // CHECK-FIXES: auto POnlyCopyCtor = std::unique_ptr<OnlyCopyCtor>(new OnlyCopyCtor{});
-
-   // Fix for classes with classes with move constructor.
-  auto POnlyCopyCtorVisible = std::unique_ptr<OnlyCopyCtorVisible>(new OnlyCopyCtorVisible{});
-  // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: use std::make_unique instead
-  // CHECK-FIXES: auto POnlyCopyCtorVisible = std::unique_ptr<OnlyCopyCtorVisible>(new OnlyCopyCtorVisible{});
-
-  auto PImplicitDeletedCopyCtor = std::unique_ptr<ImplicitDeletedCopyCtor>(new ImplicitDeletedCopyCtor{});
-  // CHECK-MESSAGES: :[[@LINE-1]]:35: warning: use std::make_unique instead
-  // CHECK-FIXES: auto PImplicitDeletedCopyCtor = std::unique_ptr<ImplicitDeletedCopyCtor>(new ImplicitDeletedCopyCtor{});
-
   // Initialization with default constructor.
   std::unique_ptr<E> PE1 = std::unique_ptr<E>(new E{});
   // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: use std::make_unique instead
   // CHECK-FIXES: std::unique_ptr<E> PE1 = std::make_unique<E>();
   PE1.reset(new E{});
   // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use std::make_unique instead
   // CHECK-FIXES: PE1 = std::make_unique<E>();