Added check uniqueptr-delete-release to replace "delete x.release()" with "x = nullptr"

[alexfh]

Nice check! One comment (quite usual ;).

test/clang-tidy/readability-uniqueptr-delete-release.cpp

25 ↗

(On Diff #35764)

I'd anchor the pattern to the start of line to verify correct replacement of "delete ".

36 ↗

(On Diff #35764)

Please add tests for code in templates with multiple instantiations and for code in macros. I suspect these cases need special treatment, which is not currently done.

[sbenza]

added more test cases and fixed the check for them.

[aaron.ballman]

clang-tidy/readability/UniqueptrDeleteReleaseCheck.h

19 ↗

(On Diff #35901)

Is there a reason for = nullptr instead of .reset(); ?

[sbenza]

clang-tidy/readability/UniqueptrDeleteReleaseCheck.h

19 ↗

(On Diff #35901)

Preference of the author and initial reviewer?
They are equivalent.

[aaron.ballman]

LGTM!

clang-tidy/readability/UniqueptrDeleteReleaseCheck.h

19 ↗

(On Diff #35901)

While equivalent, one may be considered more readable than another (in either direction!). For me, personally, I prefer seeing .reset() because it's a reminder to me that this object is also being destroyed. I don't have overly strong opinions though, and don't think it's worth some sort of user-configurability.

clang-tools-extra/trunk/clang-tidy/readability/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyReadabilityModule
   BracesAroundStatementsCheck.cpp
   ContainerSizeEmptyCheck.cpp
   ElseAfterReturnCheck.cpp
   FunctionSizeCheck.cpp
   IdentifierNamingCheck.cpp
   InconsistentDeclarationParameterNameCheck.cpp
   NamedParameterCheck.cpp
   NamespaceCommentCheck.cpp
   ReadabilityTidyModule.cpp
   RedundantStringCStrCheck.cpp
   RedundantSmartptrGetCheck.cpp
   SimplifyBooleanExprCheck.cpp
+  UniqueptrDeleteReleaseCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTooling
   )

clang-tools-extra/trunk/clang-tidy/readability/ReadabilityTidyModule.cpp

 #include "ElseAfterReturnCheck.h"
 #include "FunctionSizeCheck.h"
 #include "IdentifierNamingCheck.h"
 #include "InconsistentDeclarationParameterNameCheck.h"
 #include "NamedParameterCheck.h"
 #include "RedundantSmartptrGetCheck.h"
 #include "RedundantStringCStrCheck.h"
 #include "SimplifyBooleanExprCheck.h"
+#include "UniqueptrDeleteReleaseCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace readability {
 
 class ReadabilityModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<BracesAroundStatementsCheck>(
         "readability-braces-around-statements");
     CheckFactories.registerCheck<ContainerSizeEmptyCheck>(
         "readability-container-size-empty");
     CheckFactories.registerCheck<ElseAfterReturnCheck>(
         "readability-else-after-return");
     CheckFactories.registerCheck<FunctionSizeCheck>(
         "readability-function-size");
     CheckFactories.registerCheck<IdentifierNamingCheck>(
         "readability-identifier-naming");
     CheckFactories.registerCheck<InconsistentDeclarationParameterNameCheck>(
         "readability-inconsistent-declaration-parameter-name");
+    CheckFactories.registerCheck<UniqueptrDeleteReleaseCheck>(
+        "readability-uniqueptr-delete-release");
     CheckFactories.registerCheck<readability::NamedParameterCheck>(
         "readability-named-parameter");
     CheckFactories.registerCheck<RedundantSmartptrGetCheck>(
         "readability-redundant-smartptr-get");
     CheckFactories.registerCheck<RedundantStringCStrCheck>(
         "readability-redundant-string-cstr");
     CheckFactories.registerCheck<SimplifyBooleanExprCheck>(
         "readability-simplify-boolean-expr");

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

+//===--- UniqueptrDeleteReleaseCheck.h - clang-tidy--------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_UNIQUEPTR_DELETE_RELEASE_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_UNIQUEPTR_DELETE_RELEASE_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+
+/// Flag statements of the form: delete <unique_ptr expr>.release()
+/// and replace 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) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_UNIQUEPTR_DELETE_RELEASE_H
+

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

+//===--- UniqueptrDeleteReleaseCheck.cpp - clang-tidy----------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "UniqueptrDeleteReleaseCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+
+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")))))));
+
+  Finder->addMatcher(
+      cxxDeleteExpr(
+          has(cxxMemberCallExpr(on(expr(hasType(UniquePtrWithDefaultDelete),
+                                        unless(hasType(IsSusbstituted)))
+                                       .bind("uptr")),
+                                callee(cxxMethodDecl(hasName("release"))))))
+          .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");
+
+  if (PtrExpr->getLocStart().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->getLocEnd(), 0, *Result.SourceManager,
+                                 Result.Context->getLangOpts());
+
+  diag(DeleteExpr->getLocStart(),
+       "prefer '= nullptr' to 'delete x.release()' to reset unique_ptr<> "
+       "objects")
+      << FixItHint::CreateRemoval(CharSourceRange::getCharRange(
+             DeleteExpr->getLocStart(), PtrExpr->getLocStart()))
+      << FixItHint::CreateReplacement(
+             CharSourceRange::getTokenRange(AfterPtr, DeleteExpr->getLocEnd()),
+             " = nullptr");
+}
+
+} // namespace tidy
+} // namespace clang
+

clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst

.. toctree::
    readability-else-after-return
    readability-function-size
    readability-identifier-naming
    readability-inconsistent-declaration-parameter-name
    readability-named-parameter
    readability-redundant-smartptr-get
    readability-redundant-string-cstr
    readability-simplify-boolean-expr
+   readability-uniqueptr-delete-release

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

+readability-uniqueptr-delete-release
+====================================
+
+Replace ``delete <unique_ptr>.release()`` with ``<unique_ptr> = nullptr``.
+The latter is shorter, simpler and does not require use of raw pointer APIs.

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

+// RUN: %python %S/check_clang_tidy.py %s readability-uniqueptr-delete-release %t
+
+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();
+};
+}  // 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;
+
+  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;
+
+  delete ReturnsAUnique().release();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer '= nullptr' to 'delete
+  // CHECK-FIXES: {{^}}  ReturnsAUnique() = nullptr;
+}
+
+struct NotDefaultDeleter {};
+
+struct NotUniquePtr {
+  int* release();
+};
+
+void Negatives() {
+  std::unique_ptr<int, NotDefaultDeleter> P;
+  delete P.release();
+
+  NotUniquePtr P2;
+  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();
+
+  // We ignore this one because the deleter is a template argument.
+  // Not all instantiations will use the default deleter.
+  std::unique_ptr<int, D> P2;
+  delete P2.release();
+}
+template void NegativeDeleterT<int, std::default_delete<int>>();
+
+// Test some macros
+
+#define DELETE_RELEASE(x) delete (x).release()
+void NegativesWithTemplate() {
+  std::unique_ptr<int> P;
+  DELETE_RELEASE(P);
+}