[clang-tidy] Ignore expressions with incompatible deleters.

[sbenza]

Summary

Do not warn on .reset(.release()) expressions if the deleters are not
compatible.
Using plain assignment will probably not work.

[alexfh]

Looks good!

clang-tidy/misc/UniqueptrResetReleaseCheck.cpp

43 ↗

(On Diff #22199)

nit: put return on the next line.

50 ↗

(On Diff #22199)

nit: I'd use the concrete type here.

52 ↗

(On Diff #22199)

Can this only happen if unique_ptr is defined in a nonstandard way?

90 ↗

(On Diff #22199)

nit: Why another empty line?

[sbenza]

Minor fixes

clang-tidy/misc/UniqueptrResetReleaseCheck.cpp

43 ↗

(On Diff #22199)

Done. And another one 2 lines below. clang-format fixed it for me.

50 ↗

(On Diff #22199)

Done.

52 ↗

(On Diff #22199)

The deleter could be a function pointer.

90 ↗

(On Diff #22199)

clang-format to the rescue.

[alexfh]

clang-tidy/misc/UniqueptrResetReleaseCheck.cpp

52 ↗

(On Diff #22199)

Ah, I see. But then we can also check if the function pointers are compatible and allow the fix, can't we?

[sbenza]

Added support for function pointers.

clang-tidy/misc/UniqueptrResetReleaseCheck.cpp

52 ↗

(On Diff #22199)

Done

[sbenza]

Format fix

[alexfh]

LG

clang-tools-extra/trunk/clang-tidy/misc/UniqueptrResetReleaseCheck.cpp

 namespace tidy {
 namespace misc {
 
 void UniqueptrResetReleaseCheck::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(
       memberCallExpr(
           on(expr().bind("left")), callee(memberExpr().bind("reset_member")),
           callee(methodDecl(hasName("reset"),
-                            ofClass(hasName("::std::unique_ptr")))),
+                            ofClass(recordDecl(hasName("::std::unique_ptr"),
+                                               decl().bind("left_class"))))),
           has(memberCallExpr(
               on(expr().bind("right")),
               callee(memberExpr().bind("release_member")),
-              callee(methodDecl(hasName("release"),
-                                ofClass(hasName("::std::unique_ptr")))))))
+              callee(methodDecl(
+                  hasName("release"),
+                  ofClass(recordDecl(hasName("::std::unique_ptr"),
+                                     decl().bind("right_class"))))))))
           .bind("reset_call"),
       this);
 }
 
+namespace {
+const Type *getDeleterForUniquePtr(const MatchFinder::MatchResult &Result,
+                                   StringRef ID) {
+  const auto *Class =
+      Result.Nodes.getNodeAs<ClassTemplateSpecializationDecl>(ID);
+  if (!Class)
+    return nullptr;
+  auto DeleterArgument = Class->getTemplateArgs()[1];
+  if (DeleterArgument.getKind() != TemplateArgument::Type)
+    return nullptr;
+  return DeleterArgument.getAsType().getTypePtr();
+}
+
+bool areDeletersCompatible(const MatchFinder::MatchResult &Result) {
+  const Type *LeftDeleterType = getDeleterForUniquePtr(Result, "left_class");
+  const Type *RightDeleterType = getDeleterForUniquePtr(Result, "right_class");
+
+  if (LeftDeleterType->getUnqualifiedDesugaredType() ==
+      RightDeleterType->getUnqualifiedDesugaredType()) {
+    // Same type. We assume they are compatible.
+    // This check handles the case where the deleters are function pointers.
+    return true;
+  }
+
+  const CXXRecordDecl *LeftDeleter = LeftDeleterType->getAsCXXRecordDecl();
+  const CXXRecordDecl *RightDeleter = RightDeleterType->getAsCXXRecordDecl();
+  if (!LeftDeleter || !RightDeleter)
+    return false;
+
+  if (LeftDeleter->getCanonicalDecl() == RightDeleter->getCanonicalDecl()) {
+    // Same class. We assume they are compatible.
+    return true;
+  }
+
+  const auto *LeftAsTemplate =
+      dyn_cast<ClassTemplateSpecializationDecl>(LeftDeleter);
+  const auto *RightAsTemplate =
+      dyn_cast<ClassTemplateSpecializationDecl>(RightDeleter);
+  if (LeftAsTemplate && RightAsTemplate &&
+      LeftAsTemplate->getSpecializedTemplate() ==
+          RightAsTemplate->getSpecializedTemplate()) {
+    // They are different instantiations of the same template. We assume they
+    // are compatible.
+    // This handles things like std::default_delete<Base> vs.
+    // std::default_delete<Derived>.
+    return true;
+  }
+  return false;
+}
+
+}  // namespace
+
 void UniqueptrResetReleaseCheck::check(const MatchFinder::MatchResult &Result) {
+  if (!areDeletersCompatible(Result))
+    return;
+
   const auto *ResetMember = Result.Nodes.getNodeAs<MemberExpr>("reset_member");
   const auto *ReleaseMember =
       Result.Nodes.getNodeAs<MemberExpr>("release_member");
   const auto *Right = Result.Nodes.getNodeAs<Expr>("right");
   const auto *Left = Result.Nodes.getNodeAs<Expr>("left");
   const auto *ResetCall =
       Result.Nodes.getNodeAs<CXXMemberCallExpr>("reset_call");
 

clang-tools-extra/trunk/test/clang-tidy/misc-uniqueptr-reset-release.cpp

 // RUN: $(dirname %s)/check_clang_tidy.sh %s misc-uniqueptr-reset-release %t
 // REQUIRES: shell
 
 namespace std {
+
 template <typename T>
+struct default_delete {};
+
+template <typename T, class Deleter = std::default_delete<T>>
 struct unique_ptr {
-  unique_ptr<T>();
-  explicit unique_ptr<T>(T *);
-  template <typename U>
-  unique_ptr<T>(unique_ptr<U> &&);
+  unique_ptr();
+  explicit unique_ptr(T *);
+  template <typename U, typename E>
+  unique_ptr(unique_ptr<U, E> &&);
   void reset(T *);
   T *release();
 };
 } // namespace std
 
 struct Foo {};
 struct Bar : Foo {};
 
 std::unique_ptr<Foo> Create();
 std::unique_ptr<Foo> &Look();
 std::unique_ptr<Foo> *Get();
 
+using FooFunc = void (*)(Foo *);
+using BarFunc = void (*)(Bar *);
+
 void f() {
   std::unique_ptr<Foo> a, b;
   std::unique_ptr<Bar> c;
   std::unique_ptr<Foo> *x = &a;
   std::unique_ptr<Foo> *y = &b;
 
   a.reset(b.release());
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: prefer ptr1 = std::move(ptr2) over ptr1.reset(ptr2.release()) [misc-uniqueptr-reset-release]
   // CHECK-FIXES: a = std::move(b);
   a.reset(c.release());
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: prefer ptr1 = std::move(ptr2)
   // CHECK-FIXES: a = std::move(c);
   a.reset(Create().release());
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: prefer ptr = ReturnUnique() over ptr.reset(ReturnUnique().release()) [misc-uniqueptr-reset-release]
   // CHECK-FIXES: a = Create();
   x->reset(y->release());
   // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: prefer ptr1 = std::move(ptr2)
   // CHECK-FIXES: *x = std::move(*y);
   Look().reset(Look().release());
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: prefer ptr1 = std::move(ptr2)
   // CHECK-FIXES: Look() = std::move(Look());
   Get()->reset(Get()->release());
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: prefer ptr1 = std::move(ptr2)
   // CHECK-FIXES: *Get() = std::move(*Get());
+
+  std::unique_ptr<Bar, FooFunc> func_a, func_b;
+  func_a.reset(func_b.release());
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: prefer ptr1 = std::move(ptr2)
+  // CHECK-FIXES: func_a = std::move(func_b);
 }
 
+void negatives() {
+  std::unique_ptr<Foo> src;
+  struct OtherDeleter {};
+  std::unique_ptr<Foo, OtherDeleter> dest;
+  dest.reset(src.release());
+
+  std::unique_ptr<Bar, FooFunc> func_a;
+  std::unique_ptr<Bar, BarFunc> func_b;
+  func_a.reset(func_b.release());
+}