[clang-tools-extra] r250742 - Added check uniqueptr-delete-release to replace "delete x.release()" with "x = nullptr"
David Blaikie via cfe-commits
cfe-commits at lists.llvm.org
Tue Oct 20 11:49:23 PDT 2015
On Tue, Oct 20, 2015 at 8:39 AM, Samuel Benzaquen <sbenza at google.com> wrote:
>
> 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.
>
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 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/0388e328/attachment-0001.html>
More information about the cfe-commits
mailing list