On Mon, Oct 19, 2015 at 6:00 PM, David Blaikie <dblai...@gmail.com> wrote:
> > > On Mon, Oct 19, 2015 at 2:49 PM, Samuel Benzaquen via cfe-commits < > cfe-commits@lists.llvm.org> wrote: > >> Author: sbenza >> Date: Mon Oct 19 16:49:51 2015 >> New Revision: 250742 >> >> URL: http://llvm.org/viewvc/llvm-project?rev=250742&view=rev >> Log: >> Added check uniqueptr-delete-release to replace "delete x.release()" with >> "x = nullptr" >> > > Any stats on this? Have we seen many instances of "delete x.release()" > I would say in the hundreds. > Also, an interesting question: should the fixit be "x = nullptr" or > "x.reset()" ? I remember having this discussion with at least Lang Hames & > he preferred the latter, which I can see, though my initial reaction is to > use the former. > The opinions have been split half and half on this one, but no one feels strongly one way or another. Given that I implemented it, I went with my preference. > > >> >> Reviewers: alexfh >> >> Differential Revision: http://reviews.llvm.org/D13179 >> >> Added: >> >> clang-tools-extra/trunk/clang-tidy/readability/UniqueptrDeleteReleaseCheck.cpp >> >> clang-tools-extra/trunk/clang-tidy/readability/UniqueptrDeleteReleaseCheck.h >> >> clang-tools-extra/trunk/docs/clang-tidy/checks/readability-uniqueptr-delete-release.rst >> >> clang-tools-extra/trunk/test/clang-tidy/readability-uniqueptr-delete-release.cpp >> Modified: >> clang-tools-extra/trunk/clang-tidy/readability/CMakeLists.txt >> >> clang-tools-extra/trunk/clang-tidy/readability/ReadabilityTidyModule.cpp >> clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst >> >> Modified: clang-tools-extra/trunk/clang-tidy/readability/CMakeLists.txt >> URL: >> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/readability/CMakeLists.txt?rev=250742&r1=250741&r2=250742&view=diff >> >> ============================================================================== >> --- clang-tools-extra/trunk/clang-tidy/readability/CMakeLists.txt >> (original) >> +++ clang-tools-extra/trunk/clang-tidy/readability/CMakeLists.txt Mon Oct >> 19 16:49:51 2015 >> @@ -13,6 +13,7 @@ add_clang_library(clangTidyReadabilityMo >> RedundantStringCStrCheck.cpp >> RedundantSmartptrGetCheck.cpp >> SimplifyBooleanExprCheck.cpp >> + UniqueptrDeleteReleaseCheck.cpp >> >> LINK_LIBS >> clangAST >> >> Modified: >> clang-tools-extra/trunk/clang-tidy/readability/ReadabilityTidyModule.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/readability/ReadabilityTidyModule.cpp?rev=250742&r1=250741&r2=250742&view=diff >> >> ============================================================================== >> --- >> clang-tools-extra/trunk/clang-tidy/readability/ReadabilityTidyModule.cpp >> (original) >> +++ >> clang-tools-extra/trunk/clang-tidy/readability/ReadabilityTidyModule.cpp >> Mon Oct 19 16:49:51 2015 >> @@ -20,6 +20,7 @@ >> #include "RedundantSmartptrGetCheck.h" >> #include "RedundantStringCStrCheck.h" >> #include "SimplifyBooleanExprCheck.h" >> +#include "UniqueptrDeleteReleaseCheck.h" >> >> namespace clang { >> namespace tidy { >> @@ -40,6 +41,8 @@ public: >> "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>( >> >> Added: >> clang-tools-extra/trunk/clang-tidy/readability/UniqueptrDeleteReleaseCheck.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/readability/UniqueptrDeleteReleaseCheck.cpp?rev=250742&view=auto >> >> ============================================================================== >> --- >> clang-tools-extra/trunk/clang-tidy/readability/UniqueptrDeleteReleaseCheck.cpp >> (added) >> +++ >> clang-tools-extra/trunk/clang-tidy/readability/UniqueptrDeleteReleaseCheck.cpp >> Mon Oct 19 16:49:51 2015 >> @@ -0,0 +1,69 @@ >> +//===--- 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 >> + >> >> Added: >> clang-tools-extra/trunk/clang-tidy/readability/UniqueptrDeleteReleaseCheck.h >> URL: >> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/readability/UniqueptrDeleteReleaseCheck.h?rev=250742&view=auto >> >> ============================================================================== >> --- >> clang-tools-extra/trunk/clang-tidy/readability/UniqueptrDeleteReleaseCheck.h >> (added) >> +++ >> clang-tools-extra/trunk/clang-tidy/readability/UniqueptrDeleteReleaseCheck.h >> Mon Oct 19 16:49:51 2015 >> @@ -0,0 +1,35 @@ >> +//===--- 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 >> + >> >> Modified: clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst >> URL: >> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst?rev=250742&r1=250741&r2=250742&view=diff >> >> ============================================================================== >> --- clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst (original) >> +++ clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst Mon Oct 19 >> 16:49:51 2015 >> @@ -67,3 +67,4 @@ List of clang-tidy Checks >> readability-redundant-smartptr-get >> readability-redundant-string-cstr >> readability-simplify-boolean-expr >> + readability-uniqueptr-delete-release >> >> Added: >> clang-tools-extra/trunk/docs/clang-tidy/checks/readability-uniqueptr-delete-release.rst >> URL: >> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/clang-tidy/checks/readability-uniqueptr-delete-release.rst?rev=250742&view=auto >> >> ============================================================================== >> --- >> clang-tools-extra/trunk/docs/clang-tidy/checks/readability-uniqueptr-delete-release.rst >> (added) >> +++ >> clang-tools-extra/trunk/docs/clang-tidy/checks/readability-uniqueptr-delete-release.rst >> Mon Oct 19 16:49:51 2015 >> @@ -0,0 +1,5 @@ >> +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. >> >> Added: >> clang-tools-extra/trunk/test/clang-tidy/readability-uniqueptr-delete-release.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/readability-uniqueptr-delete-release.cpp?rev=250742&view=auto >> >> ============================================================================== >> --- >> clang-tools-extra/trunk/test/clang-tidy/readability-uniqueptr-delete-release.cpp >> (added) >> +++ >> clang-tools-extra/trunk/test/clang-tidy/readability-uniqueptr-delete-release.cpp >> Mon Oct 19 16:49:51 2015 >> @@ -0,0 +1,71 @@ >> +// 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); >> +} >> >> >> _______________________________________________ >> cfe-commits mailing list >> cfe-commits@lists.llvm.org >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >> > >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
