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

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 174760.
hokein marked 2 inline comments as done.
hokein added a comment.

Address review comments, handle move constructor as well.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54745

Files:
  clang-tidy/modernize/MakeSmartPtrCheck.cpp
  test/clang-tidy/modernize-make-unique.cpp


Index: test/clang-tidy/modernize-make-unique.cpp
===================================================================
--- test/clang-tidy/modernize-make-unique.cpp
+++ test/clang-tidy/modernize-make-unique.cpp
@@ -32,6 +32,38 @@
 
 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();
@@ -270,6 +302,33 @@
   // 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
Index: clang-tidy/modernize/MakeSmartPtrCheck.cpp
===================================================================
--- clang-tidy/modernize/MakeSmartPtrCheck.cpp
+++ clang-tidy/modernize/MakeSmartPtrCheck.cpp
@@ -375,6 +375,18 @@
       //   smart_ptr<Pair>(new Pair{first, second});
       // Has to be replaced with:
       //   smart_ptr<Pair>(Pair{first, second});
+      //
+      // The fix (std::make_unique) requires 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 auto *RD = New->getType()->getPointeeCXXRecordDecl()) {
+        for (const auto *Ctor : RD->ctors()) {
+          if (Ctor->isCopyOrMoveConstructor() &&
+              (Ctor->isDeleted() || Ctor->getAccess() == AS_private))
+            return false;
+        }
+      }
       InitRange = SourceRange(
           New->getAllocatedTypeSourceInfo()->getTypeLoc().getBeginLoc(),
           New->getInitializer()->getSourceRange().getEnd());


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D54745.174760.patch
Type: text/x-patch
Size: 4411 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20181120/96a72c02/attachment.bin>