On Tue, Oct 20, 2015 at 8:39 AM, Samuel Benzaquen <sbe...@google.com> wrote:
> > 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. > Sounds fair - thanks for the context! > > >> >> >>> >>> 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
