diff --git a/clang-tools-extra/clang-tidy/readability/UniqueptrDeleteReleaseCheck.h b/clang-tools-extra/clang-tidy/readability/UniqueptrDeleteReleaseCheck.h --- a/clang-tools-extra/clang-tidy/readability/UniqueptrDeleteReleaseCheck.h +++ b/clang-tools-extra/clang-tidy/readability/UniqueptrDeleteReleaseCheck.h @@ -22,10 +22,13 @@ /// 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 diff --git a/clang-tools-extra/clang-tidy/readability/UniqueptrDeleteReleaseCheck.cpp b/clang-tools-extra/clang-tidy/readability/UniqueptrDeleteReleaseCheck.cpp --- a/clang-tools-extra/clang-tidy/readability/UniqueptrDeleteReleaseCheck.cpp +++ b/clang-tools-extra/clang-tidy/readability/UniqueptrDeleteReleaseCheck.cpp @@ -9,6 +9,8 @@ #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; @@ -17,50 +19,69 @@ 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("uptr"); - const auto *DeleteExpr = Result.Nodes.getNodeAs("delete"); + const auto *DeleteExpr = Result.Nodes.getNodeAs("delete"); + const auto *ReleaseExpr = Result.Nodes.getNodeAs("release_expr"); + const auto *ReleaseCallExpr = + Result.Nodes.getNodeAs("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() + << FixItHint::CreateRemoval(CharSourceRange::getCharRange( + 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()), + " = nullptr"); + } } } // namespace readability diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -101,6 +101,13 @@ Added an option to choose the set of allowed functions. +- Improved :doc:`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 ----------------------------- diff --git a/clang-tools-extra/docs/clang-tidy/checks/readability-uniqueptr-delete-release.rst b/clang-tools-extra/docs/clang-tidy/checks/readability-uniqueptr-delete-release.rst --- a/clang-tools-extra/docs/clang-tidy/checks/readability-uniqueptr-delete-release.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/readability-uniqueptr-delete-release.rst @@ -15,3 +15,21 @@ std::unique_ptr 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 P; + delete P.release(); + + // becomes + + std::unique_ptr P; + P.reset(); diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability-uniqueptr-delete-release.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability-uniqueptr-delete-release.cpp --- a/clang-tools-extra/test/clang-tidy/checkers/readability-uniqueptr-delete-release.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/readability-uniqueptr-delete-release.cpp @@ -1,5 +1,6 @@ -// 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 struct default_delete {}; @@ -13,6 +14,9 @@ template unique_ptr(unique_ptr&&); T* release(); + void reset(T *P = nullptr); + T &operator*() const; + T *operator->() const; }; } // namespace std @@ -21,22 +25,62 @@ void Positives() { std::unique_ptr 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 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 *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> 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 {}; @@ -51,6 +95,9 @@ NotUniquePtr P2; delete P2.release(); + + // We don't trigger on bound member function calls. + delete (P2.release)(); } template