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 @@ -17,6 +17,16 @@ 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()))); @@ -27,11 +37,13 @@ hasName("std::default_delete"))))))); Finder->addMatcher( - cxxDeleteExpr(has(ignoringParenImpCasts(cxxMemberCallExpr( - on(expr(hasType(UniquePtrWithDefaultDelete), - unless(hasType(IsSusbstituted))) - .bind("uptr")), - callee(cxxMethodDecl(hasName("release"))))))) + cxxDeleteExpr( + has(ignoringParenImpCasts( + cxxMemberCallExpr(on(expr(hasType(UniquePtrWithDefaultDelete), + unless(hasType(IsSusbstituted))) + .bind("uptr")), + callee(cxxMethodDecl(hasName("release")))) + .bind("release_call")))) .bind("delete"), this); } @@ -40,6 +52,8 @@ const MatchFinder::MatchResult &Result) { const auto *PtrExpr = Result.Nodes.getNodeAs("uptr"); const auto *DeleteExpr = Result.Nodes.getNodeAs("delete"); + const auto *ReleaseExpr = + Result.Nodes.getNodeAs("release_call"); if (PtrExpr->getBeginLoc().isMacroID()) return; @@ -50,19 +64,22 @@ 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") + auto D = + diag(DeleteExpr->getBeginLoc(), "prefer '%select{= nullptr|.reset()}0' " + "to reset 'unique_ptr<>' objects") + << PreferResetCall << DeleteExpr->getSourceRange() << FixItHint::CreateRemoval(CharSourceRange::getCharRange( - DeleteExpr->getBeginLoc(), PtrExpr->getBeginLoc())) - << FixItHint::CreateReplacement( - CharSourceRange::getTokenRange(AfterPtr, DeleteExpr->getEndLoc()), - " = nullptr"); + DeleteExpr->getBeginLoc(), PtrExpr->getBeginLoc())); + if (PreferResetCall) { + D << FixItHint::CreateReplacement(ReleaseExpr->getExprLoc(), "reset"); + } else { + SourceLocation AfterPtr = Lexer::getLocForEndOfToken( + PtrExpr->getEndLoc(), 0, *Result.SourceManager, getLangOpts()); + D << FixItHint::CreateReplacement( + CharSourceRange::getTokenRange(AfterPtr, DeleteExpr->getEndLoc()), + " = nullptr"); + } } - } // namespace readability } // namespace tidy } // namespace clang 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,12 @@ 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``. + 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,7 @@ template unique_ptr(unique_ptr&&); T* release(); + void reset(decltype(nullptr) P = nullptr); }; } // namespace std @@ -21,22 +23,30 @@ 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(); 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(); } struct NotDefaultDeleter {};