[clang-tidy] Added option to uniqueptr delete release check

[njames93]

Summary

Adds an option, PreferResetCall, currently defaulted to false, to the check.
When true the check will refactor by calling the reset member function.

[njames93]

Clean up some fix-it logic

[Eugene.Zelenko]

clang-tools-extra/docs/clang-tidy/checks/readability-uniqueptr-delete-release.rst

24

Please use single back-ticks for option values. Same below.

[njames93]

@Eugene.Zelenko I think there should be a herald rule for marking projects as clang-tools-extra automatically

[njames93]

Use single back ticks for option values.

[njames93]

Add tests to ensure Parens are handled correctly.
Add support for pointers to and smart pointers to unique_ptr objects.

[aaron.ballman]

LGTM!

clang-tools-extra/test/clang-tidy/checkers/readability-uniqueptr-delete-release.cpp

40

Not that I expect this to come up particularly often, but, how about: delete (P2.release)();

[njames93]

clang-tools-extra/test/clang-tidy/checkers/readability-uniqueptr-delete-release.cpp

40

I don't think Im gonna worry about those given how infrequently bound member functions are used.

[aaron.ballman]

clang-tools-extra/test/clang-tidy/checkers/readability-uniqueptr-delete-release.cpp

40

I'm fine with that, but it does come up from people who loathe ADL shenanigans. That said, if it comes up *here*, I'd be surprised.

[njames93]

clang-tools-extra/test/clang-tidy/checkers/readability-uniqueptr-delete-release.cpp

40

In its current config it wont detect it at all. I got it to detect bound member functions but the fix-its never seemed to work properly. I've just left it as it. so no warning or (corrupted fix-it).

[njames93]

Add test demonstrating we aren't detecting bound member functions.

clang-tools-extra/clang-tidy/readability/UniqueptrDeleteReleaseCheck.h

 
 /// Flags statements of the form ``delete <unique_ptr expr>.release();`` and
 /// replaces them with: ``<unique_ptr expr> = nullptr;``
 ///
 /// For the user-facing documentation see:
 /// http://clang.llvm.org/extra/clang-tidy/checks/readability-uniqueptr-delete-release.html
 class UniqueptrDeleteReleaseCheck : public ClangTidyCheck {
 public:
-  UniqueptrDeleteReleaseCheck(StringRef Name, ClangTidyContext *Context)
-      : ClangTidyCheck(Name, Context) {}
+  UniqueptrDeleteReleaseCheck(StringRef Name, ClangTidyContext *Context);
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+
+private:
+  const bool PreferResetCall;
 };
 
 } // namespace readability
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_UNIQUEPTR_DELETE_RELEASE_H

clang-tools-extra/clang-tidy/readability/UniqueptrDeleteReleaseCheck.cpp

 //===--- UniqueptrDeleteReleaseCheck.cpp - clang-tidy----------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "UniqueptrDeleteReleaseCheck.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/SourceLocation.h"
 #include "clang/Lex/Lexer.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace readability {
 
+void UniqueptrDeleteReleaseCheck::storeOptions(
+    ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "PreferResetCall", PreferResetCall);
+}
+
+UniqueptrDeleteReleaseCheck::UniqueptrDeleteReleaseCheck(
+    StringRef Name, ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      PreferResetCall(Options.get("PreferResetCall", false)) {}
+
 void UniqueptrDeleteReleaseCheck::registerMatchers(MatchFinder *Finder) {
-  auto IsSusbstituted = qualType(anyOf(
-      substTemplateTypeParmType(), hasDescendant(substTemplateTypeParmType())));
 
   auto UniquePtrWithDefaultDelete = classTemplateSpecializationDecl(
-      hasName("std::unique_ptr"),
-      hasTemplateArgument(1, refersToType(qualType(hasDeclaration(cxxRecordDecl(
-                                 hasName("std::default_delete")))))));
+      hasName("::std::unique_ptr"),
+      hasTemplateArgument(1, refersToType(hasDeclaration(cxxRecordDecl(
+                                 hasName("::std::default_delete"))))));
 
   Finder->addMatcher(
-      cxxDeleteExpr(has(ignoringParenImpCasts(cxxMemberCallExpr(
-                        on(expr(hasType(UniquePtrWithDefaultDelete),
-                                unless(hasType(IsSusbstituted)))
-                               .bind("uptr")),
-                        callee(cxxMethodDecl(hasName("release")))))))
+      cxxDeleteExpr(
+          unless(isInTemplateInstantiation()),
+          has(expr(ignoringParenImpCasts(
+              cxxMemberCallExpr(
+                  callee(
+                      memberExpr(hasObjectExpression(allOf(
+                                     unless(isTypeDependent()),
+                                     anyOf(hasType(UniquePtrWithDefaultDelete),
+                                           hasType(pointsTo(
+                                               UniquePtrWithDefaultDelete))))),
+                                 member(cxxMethodDecl(hasName("release"))))
+                          .bind("release_expr")))
+                  .bind("release_call")))))
           .bind("delete"),
       this);
 }
 
 void UniqueptrDeleteReleaseCheck::check(
     const MatchFinder::MatchResult &Result) {
-  const auto *PtrExpr = Result.Nodes.getNodeAs<Expr>("uptr");
-  const auto *DeleteExpr = Result.Nodes.getNodeAs<Expr>("delete");
+  const auto *DeleteExpr = Result.Nodes.getNodeAs<CXXDeleteExpr>("delete");
+  const auto *ReleaseExpr = Result.Nodes.getNodeAs<MemberExpr>("release_expr");
+  const auto *ReleaseCallExpr =
+      Result.Nodes.getNodeAs<CXXMemberCallExpr>("release_call");
 
-  if (PtrExpr->getBeginLoc().isMacroID())
+  if (ReleaseExpr->getBeginLoc().isMacroID())
     return;
 
-  // Ignore dependent types.
-  // It can give us false positives, so we go with false negatives instead to
-  // be safe.
-  if (PtrExpr->getType()->isDependentType())
-    return;
-
-  SourceLocation AfterPtr = Lexer::getLocForEndOfToken(
-      PtrExpr->getEndLoc(), 0, *Result.SourceManager, getLangOpts());
-
-  diag(DeleteExpr->getBeginLoc(),
-       "prefer '= nullptr' to 'delete x.release()' to reset unique_ptr<> "
-       "objects")
-      << FixItHint::CreateRemoval(CharSourceRange::getCharRange(
-             DeleteExpr->getBeginLoc(), PtrExpr->getBeginLoc()))
-      << FixItHint::CreateReplacement(
-             CharSourceRange::getTokenRange(AfterPtr, DeleteExpr->getEndLoc()),
-             " = nullptr");
+  auto D =
+      diag(DeleteExpr->getBeginLoc(), "prefer '%select{= nullptr|reset()}0' "
+                                      "to reset 'unique_ptr<>' objects");
+  D << PreferResetCall << DeleteExpr->getSourceRange()
+           DeleteExpr->getBeginLoc(),
+           DeleteExpr->getArgument()->getBeginLoc()));
+  if (PreferResetCall) {
+    D << FixItHint::CreateReplacement(ReleaseExpr->getMemberLoc(), "reset");
+  } else {
+    if (ReleaseExpr->isArrow())
+      D << FixItHint::CreateInsertion(ReleaseExpr->getBase()->getBeginLoc(),
+                                      "*");
+    D << FixItHint::CreateReplacement(
+        CharSourceRange::getTokenRange(ReleaseExpr->getOperatorLoc(),
+                                       ReleaseCallExpr->getEndLoc()),
   }
+}
 
 } // namespace readability
 } // namespace tidy
 } // namespace clang

clang-tools-extra/docs/ReleaseNotes.rst

 Changes in existing checks
 ^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 - Improved :doc:`bugprone-signal-handler
   <clang-tidy/checks/bugprone-signal-handler>` check.
 
   Added an option to choose the set of allowed functions.
 
+- Improved :doc:`readability-uniqueptr-delete-release
+  <clang-tidy/checks/readability-uniqueptr-delete-release>` check.
+
+  Added an option to choose whether to refactor by calling the ``reset`` member
+  function or assignment to ``nullptr``.
+  Added support for pointers to ``std::unique_ptr``.
+
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 
 Improvements to clang-include-fixer
 -----------------------------------
 

clang-tools-extra/docs/clang-tidy/checks/readability-uniqueptr-delete-release.rst

.. code-block:: c++
 
   std::unique_ptr<int> P;
   delete P.release();
 
   // becomes
 
   std::unique_ptr<int> P;
   P = nullptr;
+  
+Options
+-------
+
+.. option:: PreferResetCall
+
+  If `true`, refactor by calling the reset member function instead of
+  assigning to ``nullptr``. Default value is `false`.
+
+  .. code-block:: c++
+
+   std::unique_ptr<int> P;
+   delete P.release();
+
+   // becomes
+
+   std::unique_ptr<int> P;
+   P.reset();

clang-tools-extra/test/clang-tidy/checkers/readability-uniqueptr-delete-release.cpp

-// RUN: %check_clang_tidy %s readability-uniqueptr-delete-release %t
-
+// RUN: %check_clang_tidy %s readability-uniqueptr-delete-release %t -check-suffix=NULLPTR
+// RUN: %check_clang_tidy %s readability-uniqueptr-delete-release %t -check-suffix=RESET -config='{ \
+// RUN: CheckOptions: [{key: readability-uniqueptr-delete-release.PreferResetCall, value: true}]}'
 namespace std {
 template <typename T>
 struct default_delete {};
 
 template <typename T, typename D = default_delete<T>>
 class unique_ptr {
  public:
   unique_ptr();
   ~unique_ptr();
   explicit unique_ptr(T*);
   template <typename U, typename E>
   unique_ptr(unique_ptr<U, E>&&);
   T* release();
+  void reset(T *P = nullptr);
+  T &operator*() const;
+  T *operator->() const;
 };
 }  // namespace std
 
 std::unique_ptr<int>& ReturnsAUnique();
 
 void Positives() {
   std::unique_ptr<int> P;
   delete P.release();
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer '= nullptr' to 'delete x.release()' to reset unique_ptr<> objects [readability-uniqueptr-delete-release]
-  // CHECK-FIXES: {{^}}  P = nullptr;
+  // CHECK-MESSAGES-NULLPTR: :[[@LINE-1]]:3: warning: prefer '= nullptr' to reset 'unique_ptr<>' objects
+  // CHECK-MESSAGES-RESET: :[[@LINE-2]]:3: warning: prefer 'reset()' to reset 'unique_ptr<>' objects
+  // CHECK-FIXES-NULLPTR: {{^}}  P = nullptr;
+  // CHECK-FIXES-RESET: {{^}}  P.reset();
 
   auto P2 = P;
   delete P2.release();
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer '= nullptr' to 'delete x.release()' to reset unique_ptr<> objects [readability-uniqueptr-delete-release]
-  // CHECK-FIXES: {{^}}  P2 = nullptr;
+  // CHECK-MESSAGES-NULLPTR: :[[@LINE-1]]:3: warning: prefer '= nullptr' to reset 'unique_ptr<>' objects
+  // CHECK-MESSAGES-RESET: :[[@LINE-2]]:3: warning: prefer 'reset()' to reset 'unique_ptr<>' objects
+  // CHECK-FIXES-NULLPTR: {{^}}  P2 = nullptr;
+  // CHECK-FIXES-RESET: {{^}}  P2.reset();
+
+  delete (P2.release());
+  // CHECK-MESSAGES-NULLPTR: :[[@LINE-1]]:3: warning: prefer '= nullptr'
+  // CHECK-MESSAGES-RESET: :[[@LINE-2]]:3: warning: prefer 'reset()'
+  // CHECK-FIXES-NULLPTR: {{^}}  (P2 = nullptr);
+  // CHECK-FIXES-RESET: {{^}}  (P2.reset());
 
   std::unique_ptr<int> Array[20];
   delete Array[4].release();
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer '= nullptr' to 'delete
-  // CHECK-FIXES: {{^}}  Array[4] = nullptr;
+  // CHECK-MESSAGES-NULLPTR: :[[@LINE-1]]:3: warning: prefer '= nullptr'
+  // CHECK-MESSAGES-RESET: :[[@LINE-2]]:3: warning: prefer 'reset()'
+  // CHECK-FIXES-NULLPTR: {{^}}  Array[4] = nullptr;
+  // CHECK-FIXES-RESET: {{^}}  Array[4].reset();
 
   delete ReturnsAUnique().release();
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer '= nullptr' to 'delete
-  // CHECK-FIXES: {{^}}  ReturnsAUnique() = nullptr;
+  // CHECK-MESSAGES-NULLPTR: :[[@LINE-1]]:3: warning: prefer '= nullptr'
+  // CHECK-MESSAGES-RESET: :[[@LINE-2]]:3: warning: prefer 'reset()'
+  // CHECK-FIXES-NULLPTR: {{^}}  ReturnsAUnique() = nullptr;
+  // CHECK-FIXES-RESET: {{^}}  ReturnsAUnique().reset();
+
+  std::unique_ptr<int> *P3(&P);
+  delete P3->release();
+  // CHECK-MESSAGES-NULLPTR: :[[@LINE-1]]:3: warning: prefer '= nullptr'
+  // CHECK-MESSAGES-RESET: :[[@LINE-2]]:3: warning: prefer 'reset()'
+  // CHECK-FIXES-NULLPTR: {{^}}  *P3 = nullptr;
+  // CHECK-FIXES-RESET: {{^}}  P3->reset();
+
+  std::unique_ptr<std::unique_ptr<int>> P4;
+  delete (*P4).release();
+  // CHECK-MESSAGES-NULLPTR: :[[@LINE-1]]:3: warning: prefer '= nullptr'
+  // CHECK-MESSAGES-RESET: :[[@LINE-2]]:3: warning: prefer 'reset()'
+  // CHECK-FIXES-NULLPTR: {{^}}  (*P4) = nullptr;
+  // CHECK-FIXES-RESET: {{^}}  (*P4).reset();
+
+  delete P4->release();
+  // CHECK-MESSAGES-NULLPTR: :[[@LINE-1]]:3: warning: prefer '= nullptr'
+  // CHECK-MESSAGES-RESET: :[[@LINE-2]]:3: warning: prefer 'reset()'
+  // CHECK-FIXES-NULLPTR: {{^}}  *P4 = nullptr;
+  // CHECK-FIXES-RESET: {{^}}  P4->reset();
+
+  delete (P4)->release();
+  // CHECK-MESSAGES-NULLPTR: :[[@LINE-1]]:3: warning: prefer '= nullptr'
+  // CHECK-MESSAGES-RESET: :[[@LINE-2]]:3: warning: prefer 'reset()'
+  // CHECK-FIXES-NULLPTR: {{^}}  *(P4) = nullptr;
+  // CHECK-FIXES-RESET: {{^}}  (P4)->reset();
 }
 
 struct NotDefaultDeleter {};
 
 struct NotUniquePtr {
   int* release();
 };
 
 void Negatives() {
   std::unique_ptr<int, NotDefaultDeleter> P;
   delete P.release();
 
   NotUniquePtr P2;
   delete P2.release();
+
+  // We don't trigger on bound member function calls.
+  delete (P2.release)();
 }
 
 template <typename T, typename D>
 void NegativeDeleterT() {
   // Ideally this would trigger a warning, but we have all dependent types
   // disabled for now.
   std::unique_ptr<T> P;
   delete P.release();