[clang-tools-extra] r250742 - Added check uniqueptr-delete-release to replace "delete x.release()" with "x = nullptr"
Samuel Benzaquen via cfe-commits
cfe-commits at lists.llvm.org
Tue Oct 20 08:39:15 PDT 2015
On Mon, Oct 19, 2015 at 6:00 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
> On Mon, Oct 19, 2015 at 2:49 PM, Samuel Benzaquen via cfe-commits <
> cfe-commits at 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 at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20151020/5e02e924/attachment-0001.html>
More information about the cfe-commits
mailing list