[clang-tidy] Don't generate incorrect fixes for class with deleted copy/move constructor in smart_ptr check.

[hokein]

Summary

The fix for aggregate initialization (std::make_unique<Foo>(Foo {1, 2}) needs
to see Foo copy/move constructor, otherwise we will have a compiler error. So we
only emit the check warning.

[JonasToth]

Does make_unique require the copy constructor if it could move?
And would the same argument apply to the move-constructors as the arguments are forwarded?
What would happen in the obscure case of a public copy-constructor, but private move-constructor (not saying it makes sense :))

clang-tidy/modernize/MakeSmartPtrCheck.cpp

380 ↗

(On Diff #174736)

typo: s/consturctor/constructor/

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

35 ↗

(On Diff #174736)

please add a case for the private copy-constructor as well.

[hokein]

Address review comments, handle move constructor as well.

[hokein]

Thanks for the review.

In D54745#1303972, @JonasToth wrote:

Does make_unique require the copy constructor if it could move?

No, in that case, move constructor will be used. I have updated the patch to include these cases.

And would the same argument apply to the move-constructors as the arguments are forwarded?
What would happen in the obscure case of a public copy-constructor, but private move-constructor (not saying it makes sense :))

This depends. The rule is complicated, for some cases it works, for other cases, it won't work.

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

35 ↗

(On Diff #174736)

Added more tests.

[JonasToth]

IMHO this patch is fine, but i think a language expert (not me :D) should take a look (@aaron.ballman ?) as its complicated :)

[aaron.ballman]

Generally LGTM with a few small nits.

clang-tidy/modernize/MakeSmartPtrCheck.cpp

379 ↗

(On Diff #174760)

requires to see -> needs to see

383 ↗

(On Diff #174760)

Don't use auto here.

384 ↗

(On Diff #174760)

How about:

if (llvm::find_if(RD->ctors(), [](const CXXConstructorDecl *CD) {
      return Ctor->isCopyOrMoveConstructor() &&
             (Ctor->isDeleted() || Ctor->getAccess() == AS_private);
    }))
  return false;

[hokein]

Address review comments.

clang-tools-extra/trunk/clang-tidy/modernize/MakeSmartPtrCheck.cpp

if (const auto *NewConstruct = New->getConstructExpr()) {
             NewConstruct->getParenOrBraceRange().getBegin().getLocWithOffset(1),
             NewConstruct->getParenOrBraceRange().getEnd().getLocWithOffset(-1));
       }
     } else {
       // Aggregate initialization.
       //   smart_ptr<Pair>(new Pair{first, second});
       // Has to be replaced with:
       //   smart_ptr<Pair>(Pair{first, second});
+      //
+      // The fix (std::make_unique) needs to see copy/move constructor of
+      // Pair. If we found any invisible or deleted copy/move constructor, we
+      // stop generating fixes -- as the C++ rule is complicated and we are less
+      // certain about the correct fixes.
+      if (const CXXRecordDecl *RD = New->getType()->getPointeeCXXRecordDecl()) {
+        if (llvm::find_if(RD->ctors(), [](const CXXConstructorDecl *Ctor) {
+              return Ctor->isCopyOrMoveConstructor() &&
+                     (Ctor->isDeleted() || Ctor->getAccess() == AS_private);
+            }) != RD->ctor_end()) {
+          return false;
+        }
+      }
       InitRange = SourceRange(
           New->getAllocatedTypeSourceInfo()->getTypeLoc().getBeginLoc(),
           New->getInitializer()->getSourceRange().getEnd());
     }
     Diag << FixItHint::CreateRemoval(
         CharSourceRange::getCharRange(NewStart, InitRange.getBegin()));
     Diag << FixItHint::CreateRemoval(
         SourceRange(InitRange.getEnd().getLocWithOffset(1), NewEnd));

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

 
 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>);
   F();
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>();