Index: clang-tidy/misc/CMakeLists.txt =================================================================== --- clang-tidy/misc/CMakeLists.txt +++ clang-tidy/misc/CMakeLists.txt @@ -27,6 +27,7 @@ SwappedArgumentsCheck.cpp ThrowByValueCatchByReferenceCheck.cpp UndelegatedConstructor.cpp + UniqueptrReleaseUnusedRetvalCheck.cpp UniqueptrResetReleaseCheck.cpp UnusedAliasDeclsCheck.cpp UnusedParametersCheck.cpp Index: clang-tidy/misc/MiscTidyModule.cpp =================================================================== --- clang-tidy/misc/MiscTidyModule.cpp +++ clang-tidy/misc/MiscTidyModule.cpp @@ -35,6 +35,7 @@ #include "ThrowByValueCatchByReferenceCheck.h" #include "UnconventionalAssignOperatorCheck.h" #include "UndelegatedConstructor.h" +#include "UniqueptrReleaseUnusedRetvalCheck.h" #include "UniqueptrResetReleaseCheck.h" #include "UnusedAliasDeclsCheck.h" #include "UnusedParametersCheck.h" @@ -94,6 +95,8 @@ "misc-throw-by-value-catch-by-reference"); CheckFactories.registerCheck( "misc-undelegated-constructor"); + CheckFactories.registerCheck( + "misc-uniqueptr-release-unused-retval"); CheckFactories.registerCheck( "misc-uniqueptr-reset-release"); CheckFactories.registerCheck( Index: clang-tidy/misc/UniqueptrReleaseUnusedRetvalCheck.h =================================================================== --- /dev/null +++ clang-tidy/misc/UniqueptrReleaseUnusedRetvalCheck.h @@ -0,0 +1,35 @@ +//===--- UniqueptrReleaseUnusedRetvalCheck.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_MISC_UNIQUEPTR_RELEASE_UNUSED_RETVAL_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_UNIQUEPTR_RELEASE_UNUSED_RETVAL_H + +#include "../ClangTidy.h" + +namespace clang { +namespace tidy { +namespace misc { + +/// This check flags calls to std::unique_ptr::release with unused return value. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/misc-uniqueptr-release-unused-retval.html +class UniqueptrReleaseUnusedRetvalCheck : public ClangTidyCheck { +public: + UniqueptrReleaseUnusedRetvalCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; +}; + +} // namespace misc +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_UNIQUEPTR_RELEASE_UNUSED_RETVAL_H Index: clang-tidy/misc/UniqueptrReleaseUnusedRetvalCheck.cpp =================================================================== --- /dev/null +++ clang-tidy/misc/UniqueptrReleaseUnusedRetvalCheck.cpp @@ -0,0 +1,42 @@ +//===--- UniqueptrReleaseUnusedRetvalCheck.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 "UniqueptrReleaseUnusedRetvalCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace misc { + +void UniqueptrReleaseUnusedRetvalCheck::registerMatchers(MatchFinder *Finder) { + // match on release() calls with CompoundStmt parent (= unused) + auto UniquePtrType = hasType(hasCanonicalType( + hasDeclaration(cxxRecordDecl(isSameOrDerivedFrom("::std::unique_ptr"))))); + auto ReleaseMethod = cxxMethodDecl(hasName("release")); + auto UnusedRetVal = hasParent(compoundStmt()); + Finder->addMatcher( + cxxMemberCallExpr(on(UniquePtrType), callee(ReleaseMethod), UnusedRetVal) + .bind("match"), + this); +} + +void UniqueptrReleaseUnusedRetvalCheck::check( + const MatchFinder::MatchResult &Result) { + if (const auto Matched = Result.Nodes.getNodeAs("match")) { + diag(Matched->getLocStart(), + "unused std::unique_ptr::release return value"); + } +} + +} // namespace misc +} // namespace tidy +} // namespace clang Index: docs/ReleaseNotes.rst =================================================================== --- docs/ReleaseNotes.rst +++ docs/ReleaseNotes.rst @@ -57,6 +57,11 @@ Improvements to clang-tidy -------------------------- +- New `misc-uniqueptr-release-unused-retval + `_ check + + Detects calls to std::unique_ptr::release where the return value is unused. + - New module `fuchsia` for Fuchsia style checks. - New module `objc` for Objective-C style checks. Index: docs/clang-tidy/checks/list.rst =================================================================== --- docs/clang-tidy/checks/list.rst +++ docs/clang-tidy/checks/list.rst @@ -142,6 +142,7 @@ misc-throw-by-value-catch-by-reference misc-unconventional-assign-operator misc-undelegated-constructor + misc-uniqueptr-release-unused-retval misc-uniqueptr-reset-release misc-unused-alias-decls misc-unused-parameters Index: docs/clang-tidy/checks/misc-uniqueptr-release-unused-retval.rst =================================================================== --- /dev/null +++ docs/clang-tidy/checks/misc-uniqueptr-release-unused-retval.rst @@ -0,0 +1,45 @@ +.. title:: clang-tidy - misc-uniqueptr-release-unused-retval + +misc-uniqueptr-release-unused-retval +==================================== + +Warns if the return value of ``std::unique_ptr::release()`` is not used. + +Discarding the return value results in leaking the managed object, if the +pointer isn't stored anywhere else. This can happen for example when +``release()`` is incorrectly used instead of ``reset()``: + +.. code-block:: c++ + + void deleteObject() { + MyUniquePtr.release(); + } + +The check will warn about this. The fix is to replace the ``release()`` call +with ``reset()``. + +Discarding the ``release()`` return value doesn't necessary result in a leak if +the pointer is also stored somewhere else: + +.. code-block:: c++ + + void f(std::unique_ptr p) { + // store the raw pointer + storePointer(p.get()); + + // prevent destroying the Foo object when the unique_ptr is destructed + p.release(); + } + +The check warns also here. Although there's no leak here, the code can still be +improved by using the ``release()`` return value: + +.. code-block:: c++ + + void f(std::unique_ptr p) { + storePointer(p.release()); + } + +This eliminates the possibility that code causing ``f()`` to return, thus +causing ``p``'s destructor to be called and making the stored raw pointer +dangle, is added between ``storePointer()`` and ``release()`` calls. Index: test/clang-tidy/misc-uniqueptr-release-unused-retval.cpp =================================================================== --- /dev/null +++ test/clang-tidy/misc-uniqueptr-release-unused-retval.cpp @@ -0,0 +1,49 @@ +// RUN: %check_clang_tidy %s misc-uniqueptr-release-unused-retval %t + +namespace std { +template +struct unique_ptr { + T *operator->(); + T *release(); +}; +} // namespace std + +struct Foo { + int release(); +}; + +template +void callRelease(T &t) { t.release(); } +// CHECK-MESSAGES: [[@LINE-1]]:26: warning: unused std::unique_ptr::release return value [misc-uniqueptr-release-unused-retval] + +using FooPtr = std::unique_ptr; + +template +struct Derived : public std::unique_ptr { +}; + +void deleteThis(Foo *pointer) { delete pointer; } + +void Warning() { + std::unique_ptr p1; + p1.release(); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: unused std::unique_ptr::release return value [misc-uniqueptr-release-unused-retval] + { p1.release(); } + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: unused std::unique_ptr::release return value [misc-uniqueptr-release-unused-retval] + callRelease(p1); + FooPtr fp; + fp.release(); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: unused std::unique_ptr::release return value [misc-uniqueptr-release-unused-retval] + Derived dp; + dp.release(); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: unused std::unique_ptr::release return value [misc-uniqueptr-release-unused-retval] +} + +void NoWarning() { + std::unique_ptr p2; + auto q = p2.release(); + delete p2.release(); + deleteThis(p2.release()); + p2->release(); + p2.release()->release(); +}