https://github.com/pizzud updated https://github.com/llvm/llvm-project/pull/67467
>From 04a3e8d8cbd6943f44a81fddb0524902202a1a78 Mon Sep 17 00:00:00 2001 From: David Pizzuto <piz...@google.com> Date: Tue, 26 Sep 2023 10:45:42 -0700 Subject: [PATCH 1/6] [clang-tidy] Add bugprone-move-shared-pointer-contents check. This check detects moves of the contents of a shared pointer rather than the pointer itself. Other code with a reference to the shared pointer is probably not expecting the move. The set of shared pointer classes is configurable via options to allow individual projects to cover additional types. --- .../MoveSharedPointerContentsCheck.cpp | 157 ++++++++++++++++++ .../bugprone/MoveSharedPointerContentsCheck.h | 45 +++++ .../bugprone/move-shared-pointer-contents.rst | 19 +++ .../docs/clang-tidy/checks/list.rst | 4 + 4 files changed, 225 insertions(+) create mode 100644 clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.cpp create mode 100644 clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.h create mode 100644 clang-tools-extra/docs/clang-tidy/checks/bugprone/move-shared-pointer-contents.rst diff --git a/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.cpp new file mode 100644 index 0000000000000..461fe91267e63 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.cpp @@ -0,0 +1,157 @@ +//===--- MoveSharedPointerContentsCheck.cpp - clang-tidy ------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "MoveSharedPointerContentsCheck.h" +#include "../ClangTidyCheck.h" +#include "../utils/Matchers.h" +#include "../utils/OptionsUtils.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; + +namespace clang::tidy::bugprone { + +MoveSharedPointerContentsCheck::MoveSharedPointerContentsCheck( + StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + SharedPointerClasses(utils::options::parseStringList( + Options.get("SharedPointerClasses", "::std::shared_ptr"))) {} + +void MoveSharedPointerContentsCheck::registerMatchers(MatchFinder *Finder) { + auto isStdMove = callee(functionDecl(hasName("::std::move"))); + + // Resolved type, direct move. + Finder->addMatcher( + callExpr(isStdMove, hasArgument(0, cxxOperatorCallExpr( + hasOverloadedOperatorName("*"), + callee(cxxMethodDecl(ofClass( + matchers::matchesAnyListedName( + SharedPointerClasses))))))) + .bind("call"), + this); + + // Resolved type, move out of get(). + Finder->addMatcher( + callExpr( + isStdMove, + hasArgument( + 0, unaryOperator( + hasOperatorName("*"), + hasUnaryOperand(cxxMemberCallExpr(callee(cxxMethodDecl( + hasName("get"), ofClass(matchers::matchesAnyListedName( + SharedPointerClasses))))))))) + .bind("get_call"), + this); + + auto isStdMoveUnresolved = callee(unresolvedLookupExpr( + hasAnyDeclaration(namedDecl(hasUnderlyingDecl(hasName("::std::move")))))); + + // Unresolved type, direct move. + Finder->addMatcher( + callExpr( + isStdMoveUnresolved, + hasArgument(0, unaryOperator(hasOperatorName("*"), + hasUnaryOperand(declRefExpr(hasType( + qualType().bind("unresolved_p"))))))) + .bind("unresolved_call"), + this); + // Annoyingly, the declRefExpr in the unresolved-move-of-get() case + // is of <dependent type> rather than shared_ptr<T>, so we have to + // just fetch the variable. This does leave a gap where a temporary + // shared_ptr wouldn't be caught, but moving out of a temporary + // shared pointer is a truly wild thing to do so it should be okay. + + // Unresolved type, move out of get(). + Finder->addMatcher( + callExpr(isStdMoveUnresolved, + hasArgument( + 0, unaryOperator(hasOperatorName("*"), + hasDescendant(cxxDependentScopeMemberExpr( + hasMemberName("get"))), + hasDescendant(declRefExpr(to( + varDecl().bind("unresolved_get_p"))))))) + .bind("unresolved_get_call"), + this); +} + +bool MoveSharedPointerContentsCheck::isSharedPointerClass( + const VarDecl *VD) const { + if (VD == nullptr) + return false; + + const QualType QT = VD->getType(); + return isSharedPointerClass(&QT); +} + +bool MoveSharedPointerContentsCheck::isSharedPointerClass( + const QualType *QT) const { + if (QT == nullptr) + return false; + + // We want the qualified name without template parameters, + // const/volatile, or reference/pointer qualifiers so we can look + // it up in SharedPointerClasses. This is a bit messy, but gets us + // to the underlying type without template parameters (eg + // std::shared_ptr) or const/volatile qualifiers even in the face of + // typedefs. + + bool found = false; + const auto *Template = llvm::dyn_cast<TemplateSpecializationType>( + QT->getSplitDesugaredType().Ty); + if (Template != nullptr) { + const std::string TypeName = Template->getTemplateName() + .getAsTemplateDecl() + ->getQualifiedNameAsString(); + for (const llvm::StringRef SharedPointer : SharedPointerClasses) { + // SharedPointer entries may or may not have leading ::, but TypeName + // definitely won't. + if (SharedPointer == TypeName || SharedPointer.substr(2) == TypeName) { + found = true; + break; + } + } + } + + return found; +} + +void MoveSharedPointerContentsCheck::check( + const MatchFinder::MatchResult &Result) { + const bool Unresolved = + isSharedPointerClass(Result.Nodes.getNodeAs<QualType>("unresolved_p")); + const bool UnresolvedGet = + isSharedPointerClass(Result.Nodes.getNodeAs<VarDecl>("unresolved_get_p")); + + clang::SourceLocation Loc; + if (const auto *UnresolvedCall = + Result.Nodes.getNodeAs<CallExpr>("unresolved_call"); + UnresolvedCall != nullptr && Unresolved) { + Loc = UnresolvedCall->getBeginLoc(); + } else if (const auto *UnresolvedGetCall = + Result.Nodes.getNodeAs<CallExpr>("unresolved_get_call"); + UnresolvedGetCall != nullptr && UnresolvedGet) { + Loc = UnresolvedGetCall->getBeginLoc(); + } else if (const auto *GetCall = Result.Nodes.getNodeAs<CallExpr>("get_call"); + GetCall != nullptr) { + Loc = GetCall->getBeginLoc(); + } else if (const auto *Call = Result.Nodes.getNodeAs<CallExpr>("call"); + Call != nullptr) { + Loc = Call->getBeginLoc(); + } else { + return; + } + + if (Loc.isValid()) { + diag(Loc, + "don't move the contents out of a shared pointer, as other accessors " + "expect them to remain in a determinate state"); + } +} + +} // namespace clang::tidy::bugprone diff --git a/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.h b/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.h new file mode 100644 index 0000000000000..c3a6ca83b24ec --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.h @@ -0,0 +1,45 @@ +//===--- MoveSharedPointerContentsCheck.h - clang-tidy ----------*- C++ -*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_MOVESHAREDPOINTERCONTENTSCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_MOVESHAREDPOINTERCONTENTSCHECK_H + +#include <vector> + +#include "../ClangTidyCheck.h" + +namespace clang::tidy::bugprone { + +/// Detects calls to move the contents out of a ``std::shared_ptr`` rather than +/// moving the pointer itself. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/move-shared-pointer-contents.html +class MoveSharedPointerContentsCheck : public ClangTidyCheck { +public: + MoveSharedPointerContentsCheck(StringRef Name, ClangTidyContext *Context); + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + bool + isLanguageVersionSupported(const LangOptions &LangOptions) const override { + return LangOptions.CPlusPlus11; + } + std::optional<TraversalKind> getCheckTraversalKind() const override { + return TK_IgnoreUnlessSpelledInSource; + } + +private: + // Returns whether the type or variable is one of the SharedPointerClasses. + bool isSharedPointerClass(const VarDecl *VD) const; + bool isSharedPointerClass(const QualType *QT) const; + const std::vector<StringRef> SharedPointerClasses; +}; + +} // namespace clang::tidy::bugprone + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_MOVESHAREDPOINTERCONTENTSCHECK_H diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/move-shared-pointer-contents.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/move-shared-pointer-contents.rst new file mode 100644 index 0000000000000..ea4c1fb6df708 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/move-shared-pointer-contents.rst @@ -0,0 +1,19 @@ +.. title:: clang-tidy - bugprone-move-shared-pointer-contents + +bugprone-move-shared-pointer-contents +===================================== + + +Detects calls to move the contents out of a ``std::shared_ptr`` rather +than moving the pointer itself. In other words, calling +``std::move(*p)`` or ``std::move(*p.get())``. Other reference holders +may not be expecting the move and suddenly getting empty or otherwise +indeterminate states can cause issues. + +Options +------- +.. option :: SharedPointerClasses + + A semicolon-separated list of class names that should be treated as shared + pointers. Classes are resolved through aliases, so any alias to the defined + classes will be considered. Default is `::std::shared_ptr`. diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index 2f86121ad8729..a2996b9f9f18f 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -118,6 +118,10 @@ Clang-Tidy Checks :doc:`bugprone-posix-return <bugprone/posix-return>`, "Yes" :doc:`bugprone-redundant-branch-condition <bugprone/redundant-branch-condition>`, "Yes" :doc:`bugprone-reserved-identifier <bugprone/reserved-identifier>`, "Yes" +<<<<<<< HEAD +======= + :doc:`bugprone-shared-pointer-contents-move <bugprone/shared-pointer-contents-move>`, +>>>>>>> b81b29d180ac ([clang-tidy] Add bugprone-move-shared-pointer-contents check.) :doc:`bugprone-shared-ptr-array-mismatch <bugprone/shared-ptr-array-mismatch>`, "Yes" :doc:`bugprone-signal-handler <bugprone/signal-handler>`, :doc:`bugprone-signed-char-misuse <bugprone/signed-char-misuse>`, >From c11863e9a57ff7b4aa10cdcc91f0294b12b692f7 Mon Sep 17 00:00:00 2001 From: David Pizzuto <piz...@google.com> Date: Tue, 26 Sep 2023 10:45:42 -0700 Subject: [PATCH 2/6] [clang-tidy] Add bugprone-move-shared-pointer-contents check. This check detects moves of the contents of a shared pointer rather than the pointer itself. Other code with a reference to the shared pointer is probably not expecting the move. The set of shared pointer classes is configurable via options to allow individual projects to cover additional types. --- .../bugprone/BugproneTidyModule.cpp | 3 + .../clang-tidy/bugprone/CMakeLists.txt | 1 + clang-tools-extra/docs/ReleaseNotes.rst | 11 +- .../bugprone/move-shared-pointer-contents.cpp | 125 ++++++++++++++++++ 4 files changed, 137 insertions(+), 3 deletions(-) create mode 100644 clang-tools-extra/test/clang-tidy/checkers/bugprone/move-shared-pointer-contents.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp index 435cb1e3fbcff..4e4092b90d4cf 100644 --- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp @@ -41,6 +41,7 @@ #include "MisplacedPointerArithmeticInAllocCheck.h" #include "MisplacedWideningCastCheck.h" #include "MoveForwardingReferenceCheck.h" +#include "MoveSharedPointerContentsCheck.h" #include "MultiLevelImplicitPointerConversionCheck.h" #include "MultipleNewInOneExpressionCheck.h" #include "MultipleStatementMacroCheck.h" @@ -155,6 +156,8 @@ class BugproneModule : public ClangTidyModule { "bugprone-misplaced-widening-cast"); CheckFactories.registerCheck<MoveForwardingReferenceCheck>( "bugprone-move-forwarding-reference"); + CheckFactories.registerCheck<MoveSharedPointerContentsCheck>( + "bugprone-move-shared-pointer-contents"); CheckFactories.registerCheck<MultiLevelImplicitPointerConversionCheck>( "bugprone-multi-level-implicit-pointer-conversion"); CheckFactories.registerCheck<MultipleNewInOneExpressionCheck>( diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt index 70e7fbc7ec0c1..62bba10ad94dc 100644 --- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt @@ -37,6 +37,7 @@ add_clang_library(clangTidyBugproneModule MisplacedPointerArithmeticInAllocCheck.cpp MisplacedWideningCastCheck.cpp MoveForwardingReferenceCheck.cpp + MoveSharedPointerContentsCheck.cpp MultiLevelImplicitPointerConversionCheck.cpp MultipleNewInOneExpressionCheck.cpp MultipleStatementMacroCheck.cpp diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index b4d87e0ed2a67..779b7d66f8d05 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -162,6 +162,12 @@ New checks Detects incorrect usages of ``std::enable_if`` that don't name the nested ``type`` type. +- New :doc:`bugprone-move-shared-pointer-contents + <clang-tidy/checks/bugprone/move-shared-pointer-contents>` check. + + Detects calls to move the contents out of a ``std::shared_ptr`` rather than + moving the pointer itself. + - New :doc:`bugprone-multi-level-implicit-pointer-conversion <clang-tidy/checks/bugprone/multi-level-implicit-pointer-conversion>` check. @@ -368,8 +374,7 @@ Changes in existing checks <clang-tidy/checks/misc/const-correctness>` check to avoid false positive when using pointer to member function. Additionally, the check no longer emits a diagnostic when a variable that is not type-dependent is an operand of a - type-dependent binary operator. Improved performance of the check through - optimizations. + type-dependent binary operator. - Improved :doc:`misc-include-cleaner <clang-tidy/checks/misc/include-cleaner>` check by adding option @@ -383,7 +388,7 @@ Changes in existing checks - Improved :doc:`misc-unused-using-decls <clang-tidy/checks/misc/unused-using-decls>` check to avoid false positive when - using in elaborated type and only check cpp files. + using in elaborated type. - Improved :doc:`modernize-avoid-bind <clang-tidy/checks/modernize/avoid-bind>` check to diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/move-shared-pointer-contents.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/move-shared-pointer-contents.cpp new file mode 100644 index 0000000000000..ccc4400aa691c --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/move-shared-pointer-contents.cpp @@ -0,0 +1,125 @@ +// RUN: %check_clang_tidy %s bugprone-move-shared-pointer-contents %t -- -config="{CheckOptions: {bugprone-move-shared-pointer-contents.SharedPointerClasses: '::std::shared_ptr;my::OtherSharedPtr;'}}" + +// Some dummy definitions we'll need. + +namespace std { + +using size_t = int; + +template <typename> struct remove_reference; +template <typename _Tp> struct remove_reference { typedef _Tp type; }; +template <typename _Tp> struct remove_reference<_Tp &> { typedef _Tp type; }; +template <typename _Tp> struct remove_reference<_Tp &&> { typedef _Tp type; }; + +template <typename _Tp> +constexpr typename std::remove_reference<_Tp>::type &&move(_Tp &&__t) { + return static_cast<typename std::remove_reference<_Tp>::type &&>(__t); +} + +template <typename T> +struct shared_ptr { + shared_ptr(); + T *get() const; + explicit operator bool() const; + void reset(T *ptr); + T &operator*() const; + T *operator->() const; +}; + +} // namespace std + +namespace my { +template <typename T> +using OtherSharedPtr = std::shared_ptr<T>; +// Not part of the config. +template <typename T> +using YetAnotherSharedPtr = T*; +} // namespace my + +struct Nontrivial { + int x; + Nontrivial() : x(2) {} + Nontrivial(Nontrivial& other) { x = other.x; } + Nontrivial(Nontrivial&& other) { x = std::move(other.x); } + Nontrivial& operator=(Nontrivial& other) { x = other.x; } + Nontrivial& operator=(Nontrivial&& other) { x = std::move(other.x); } +}; + +// Test cases begin here. + +void correct() { + std::shared_ptr<Nontrivial> p; + Nontrivial x = *std::move(p); +} + +void simpleFinding() { + std::shared_ptr<Nontrivial> p; + Nontrivial y = std::move(*p); +} +// CHECK-MESSAGES: :[[@LINE-2]]:18: warning: don't move the contents out of a shared pointer, as other accessors expect them to remain in a determinate state [bugprone-move-shared-pointer-contents] + +void aliasedType() { + using nontrivial_ptr = std::shared_ptr<Nontrivial>; + nontrivial_ptr p; + Nontrivial x = std::move(*p); +} +// CHECK-MESSAGES: :[[@LINE-2]]:18: warning: don't move the contents out of a shared pointer, as other accessors expect them to remain in a determinate state [bugprone-move-shared-pointer-contents] + +void configWorks() { + my::OtherSharedPtr<Nontrivial> p; + Nontrivial x = std::move(*p); +} +// CHECK-MESSAGES: :[[@LINE-2]]:18: warning: don't move the contents out of a shared pointer, as other accessors expect them to remain in a determinate state [bugprone-move-shared-pointer-contents] + +void sharedEsquePointerNotInConfig() { + my::YetAnotherSharedPtr<Nontrivial> p; + Nontrivial x = std::move(*p); +} + + +void multiStars() { + std::shared_ptr<Nontrivial> p; + int x = 2 * std::move(*p).x * 3; +} +// CHECK-MESSAGES: :[[@LINE-2]]:15: warning: don't move the contents out of a shared pointer, as other accessors expect them to remain in a determinate state [bugprone-move-shared-pointer-contents] + +template <typename T> +void unresolved() { + std::shared_ptr<T> p; + int x = 2 * std::move(*p).x * 3; +} +// CHECK-MESSAGES: :[[@LINE-2]]:15: warning: don't move the contents out of a shared pointer, as other accessors expect them to remain in a determinate state [bugprone-move-shared-pointer-contents] + +template <typename T> +void unresolvedAsAReference() { + std::shared_ptr<T> p; + std::shared_ptr<T>& q = p; + int x = 2 * std::move(*q).x * 3; +} +// CHECK-MESSAGES: :[[@LINE-2]]:15: warning: don't move the contents out of a shared pointer, as other accessors expect them to remain in a determinate state [bugprone-move-shared-pointer-contents] + +template <typename T> +void unresolvedAlias() { + my::OtherSharedPtr<T> p; + Nontrivial x = std::move(*p); +} +// CHECK-MESSAGES: :[[@LINE-2]]:18: warning: don't move the contents out of a shared pointer, as other accessors expect them to remain in a determinate state [bugprone-move-shared-pointer-contents] + +void dereferencedGet() { + std::shared_ptr<Nontrivial> p; + Nontrivial x = std::move(*p.get()); +} +// CHECK-MESSAGES: :[[@LINE-2]]:18: warning: don't move the contents out of a shared pointer, as other accessors expect them to remain in a determinate state [bugprone-move-shared-pointer-contents] + +template <typename T> +void unresolvedDereferencedGet() { + std::shared_ptr<T> p; + T x = std::move(*p.get()); +} +// CHECK-MESSAGES: :[[@LINE-2]]:9: warning: don't move the contents out of a shared pointer, as other accessors expect them to remain in a determinate state [bugprone-move-shared-pointer-contents] + +template <typename T> +void rawPointer() { + T* p; + T x = std::move(*p); +} >From da200c7db6f4be6c2fc882e19db2275e3686de07 Mon Sep 17 00:00:00 2001 From: David Pizzuto <piz...@google.com> Date: Tue, 26 Sep 2023 10:45:42 -0700 Subject: [PATCH 3/6] [clang-tidy] Add bugprone-move-shared-pointer-contents check. This check detects moves of the contents of a shared pointer rather than the pointer itself. Other code with a reference to the shared pointer is probably not expecting the move. The set of shared pointer classes is configurable via options to allow individual projects to cover additional types. --- clang-tools-extra/docs/ReleaseNotes.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 779b7d66f8d05..1238ef8336239 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -167,7 +167,7 @@ New checks Detects calls to move the contents out of a ``std::shared_ptr`` rather than moving the pointer itself. - + - New :doc:`bugprone-multi-level-implicit-pointer-conversion <clang-tidy/checks/bugprone/multi-level-implicit-pointer-conversion>` check. >From 3cae8d53420fb18eafc5892fbf6d5acce4d57550 Mon Sep 17 00:00:00 2001 From: David Pizzuto <piz...@google.com> Date: Tue, 26 Sep 2023 10:45:42 -0700 Subject: [PATCH 4/6] [clang-tidy] Add bugprone-move-shared-pointer-contents check. This check detects moves of the contents of a shared pointer rather than the pointer itself. Other code with a reference to the shared pointer is probably not expecting the move. The set of shared pointer classes is configurable via options to allow individual projects to cover additional types. --- clang-tools-extra/docs/ReleaseNotes.rst | 2 +- clang-tools-extra/docs/clang-tidy/checks/list.rst | 5 +---- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 779b7d66f8d05..1238ef8336239 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -167,7 +167,7 @@ New checks Detects calls to move the contents out of a ``std::shared_ptr`` rather than moving the pointer itself. - + - New :doc:`bugprone-multi-level-implicit-pointer-conversion <clang-tidy/checks/bugprone/multi-level-implicit-pointer-conversion>` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index a2996b9f9f18f..b7689889dd283 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -117,11 +117,8 @@ Clang-Tidy Checks :doc:`bugprone-parent-virtual-call <bugprone/parent-virtual-call>`, "Yes" :doc:`bugprone-posix-return <bugprone/posix-return>`, "Yes" :doc:`bugprone-redundant-branch-condition <bugprone/redundant-branch-condition>`, "Yes" - :doc:`bugprone-reserved-identifier <bugprone/reserved-identifier>`, "Yes" -<<<<<<< HEAD -======= + :doc:`bugprone-reserved-identifier <bugprone/reserved-identifier>`, "Yes", :doc:`bugprone-shared-pointer-contents-move <bugprone/shared-pointer-contents-move>`, ->>>>>>> b81b29d180ac ([clang-tidy] Add bugprone-move-shared-pointer-contents check.) :doc:`bugprone-shared-ptr-array-mismatch <bugprone/shared-ptr-array-mismatch>`, "Yes" :doc:`bugprone-signal-handler <bugprone/signal-handler>`, :doc:`bugprone-signed-char-misuse <bugprone/signed-char-misuse>`, >From dfd266619b48f22afa6848130750157782dbdecd Mon Sep 17 00:00:00 2001 From: David Pizzuto <piz...@google.com> Date: Mon, 1 Apr 2024 13:18:08 -0700 Subject: [PATCH 5/6] This check detects moves of the contents of a shared pointer rather than the pointer itself. Other code with a reference to the shared pointer is probably not expecting the move. The set of shared pointer classes is configurable via options to allow individual projects to cover additional types. --- .../MoveSharedPointerContentsCheck.cpp | 158 ++++++++---------- .../bugprone/MoveSharedPointerContentsCheck.h | 3 - .../bugprone/move-shared-pointer-contents.cpp | 13 ++ 3 files changed, 86 insertions(+), 88 deletions(-) diff --git a/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.cpp index 461fe91267e63..9ff357b58f977 100644 --- a/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.cpp @@ -6,6 +6,9 @@ // //===----------------------------------------------------------------------===// +#include <signal.h> +#include <unistd.h> + #include "MoveSharedPointerContentsCheck.h" #include "../ClangTidyCheck.h" #include "../utils/Matchers.h" @@ -16,6 +19,30 @@ using namespace clang::ast_matchers; namespace clang::tidy::bugprone { +namespace { + +// Reports whether the QualType matches the inner matcher, which is expected to be +// matchesAnyListedName. The QualType is expected to either point to a RecordDecl +// (for concrete types) or an ElaboratedType (for dependent ones). +AST_MATCHER_P(QualType, isSharedPointer, + clang::ast_matchers::internal::Matcher<NamedDecl>, InnerMatcher) { + if (const auto *RD = Node.getTypePtr()->getAsCXXRecordDecl(); RD != nullptr) { + return InnerMatcher.matches(*RD, Finder, Builder); + } else if (const auto *ED = Node.getTypePtr()->getAs<ElaboratedType>(); + ED != nullptr) { + if (const auto *TS = ED->getNamedType() + .getTypePtr() + ->getAs<TemplateSpecializationType>(); + TS != nullptr) { + return InnerMatcher.matches(*TS->getTemplateName().getAsTemplateDecl(), + Finder, Builder); + } + } + + return false; +} + +} // namespace MoveSharedPointerContentsCheck::MoveSharedPointerContentsCheck( StringRef Name, ClangTidyContext *Context) @@ -26,116 +53,77 @@ MoveSharedPointerContentsCheck::MoveSharedPointerContentsCheck( void MoveSharedPointerContentsCheck::registerMatchers(MatchFinder *Finder) { auto isStdMove = callee(functionDecl(hasName("::std::move"))); - // Resolved type, direct move. - Finder->addMatcher( - callExpr(isStdMove, hasArgument(0, cxxOperatorCallExpr( - hasOverloadedOperatorName("*"), - callee(cxxMethodDecl(ofClass( - matchers::matchesAnyListedName( - SharedPointerClasses))))))) + auto resolvedType = callExpr(anyOf( + // Resolved type, direct move. + callExpr( + isStdMove, + hasArgument( + 0, + cxxOperatorCallExpr( + hasOverloadedOperatorName("*"), + hasDescendant(declRefExpr(hasType(qualType(isSharedPointer( + matchers::matchesAnyListedName(SharedPointerClasses)))))), + callee(cxxMethodDecl())))) .bind("call"), - this); - - // Resolved type, move out of get(). - Finder->addMatcher( + // Resolved type, move out of get(). callExpr( isStdMove, hasArgument( 0, unaryOperator( hasOperatorName("*"), - hasUnaryOperand(cxxMemberCallExpr(callee(cxxMethodDecl( - hasName("get"), ofClass(matchers::matchesAnyListedName( - SharedPointerClasses))))))))) - .bind("get_call"), - this); + hasUnaryOperand(allOf( + hasDescendant(declRefExpr(hasType(qualType( + isSharedPointer(matchers::matchesAnyListedName( + SharedPointerClasses)))))), + cxxMemberCallExpr( + callee(cxxMethodDecl(hasName("get"))))))))) + .bind("get_call"))); + + Finder->addMatcher(resolvedType, this); auto isStdMoveUnresolved = callee(unresolvedLookupExpr( hasAnyDeclaration(namedDecl(hasUnderlyingDecl(hasName("::std::move")))))); - // Unresolved type, direct move. - Finder->addMatcher( + auto unresolvedType = callExpr(anyOf( + // Unresolved type, direct move. callExpr( isStdMoveUnresolved, - hasArgument(0, unaryOperator(hasOperatorName("*"), - hasUnaryOperand(declRefExpr(hasType( - qualType().bind("unresolved_p"))))))) + hasArgument(0, unaryOperator( + hasOperatorName("*"), + hasUnaryOperand(declRefExpr(hasType(qualType( + isSharedPointer(matchers::matchesAnyListedName( + SharedPointerClasses))))))))) .bind("unresolved_call"), - this); - // Annoyingly, the declRefExpr in the unresolved-move-of-get() case - // is of <dependent type> rather than shared_ptr<T>, so we have to - // just fetch the variable. This does leave a gap where a temporary - // shared_ptr wouldn't be caught, but moving out of a temporary - // shared pointer is a truly wild thing to do so it should be okay. - - // Unresolved type, move out of get(). - Finder->addMatcher( + + // Annoyingly, the declRefExpr in the unresolved-move-of-get() case + // is of <dependent type> rather than shared_ptr<T>, so we have to + // just fetch the variable. This does leave a gap where a temporary + // shared_ptr wouldn't be caught, but moving out of a temporary + // shared pointer is a truly wild thing to do so it should be okay. callExpr(isStdMoveUnresolved, hasArgument( - 0, unaryOperator(hasOperatorName("*"), - hasDescendant(cxxDependentScopeMemberExpr( - hasMemberName("get"))), - hasDescendant(declRefExpr(to( - varDecl().bind("unresolved_get_p"))))))) - .bind("unresolved_get_call"), - this); -} - -bool MoveSharedPointerContentsCheck::isSharedPointerClass( - const VarDecl *VD) const { - if (VD == nullptr) - return false; - - const QualType QT = VD->getType(); - return isSharedPointerClass(&QT); -} - -bool MoveSharedPointerContentsCheck::isSharedPointerClass( - const QualType *QT) const { - if (QT == nullptr) - return false; - - // We want the qualified name without template parameters, - // const/volatile, or reference/pointer qualifiers so we can look - // it up in SharedPointerClasses. This is a bit messy, but gets us - // to the underlying type without template parameters (eg - // std::shared_ptr) or const/volatile qualifiers even in the face of - // typedefs. - - bool found = false; - const auto *Template = llvm::dyn_cast<TemplateSpecializationType>( - QT->getSplitDesugaredType().Ty); - if (Template != nullptr) { - const std::string TypeName = Template->getTemplateName() - .getAsTemplateDecl() - ->getQualifiedNameAsString(); - for (const llvm::StringRef SharedPointer : SharedPointerClasses) { - // SharedPointer entries may or may not have leading ::, but TypeName - // definitely won't. - if (SharedPointer == TypeName || SharedPointer.substr(2) == TypeName) { - found = true; - break; - } - } - } - - return found; + 0, unaryOperator( + hasOperatorName("*"), + hasDescendant(cxxDependentScopeMemberExpr( + hasMemberName("get"))), + hasDescendant(declRefExpr(to(varDecl(hasType(qualType( + isSharedPointer(matchers::matchesAnyListedName( + SharedPointerClasses))))))))))) + .bind("unresolved_get_call"))); + + Finder->addMatcher(unresolvedType, this); } void MoveSharedPointerContentsCheck::check( const MatchFinder::MatchResult &Result) { - const bool Unresolved = - isSharedPointerClass(Result.Nodes.getNodeAs<QualType>("unresolved_p")); - const bool UnresolvedGet = - isSharedPointerClass(Result.Nodes.getNodeAs<VarDecl>("unresolved_get_p")); - clang::SourceLocation Loc; if (const auto *UnresolvedCall = Result.Nodes.getNodeAs<CallExpr>("unresolved_call"); - UnresolvedCall != nullptr && Unresolved) { + UnresolvedCall != nullptr) { Loc = UnresolvedCall->getBeginLoc(); } else if (const auto *UnresolvedGetCall = Result.Nodes.getNodeAs<CallExpr>("unresolved_get_call"); - UnresolvedGetCall != nullptr && UnresolvedGet) { + UnresolvedGetCall != nullptr) { Loc = UnresolvedGetCall->getBeginLoc(); } else if (const auto *GetCall = Result.Nodes.getNodeAs<CallExpr>("get_call"); GetCall != nullptr) { diff --git a/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.h b/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.h index c3a6ca83b24ec..d2a42c49ddf67 100644 --- a/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.h +++ b/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.h @@ -34,9 +34,6 @@ class MoveSharedPointerContentsCheck : public ClangTidyCheck { } private: - // Returns whether the type or variable is one of the SharedPointerClasses. - bool isSharedPointerClass(const VarDecl *VD) const; - bool isSharedPointerClass(const QualType *QT) const; const std::vector<StringRef> SharedPointerClasses; }; diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/move-shared-pointer-contents.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/move-shared-pointer-contents.cpp index ccc4400aa691c..8a741581c3092 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/move-shared-pointer-contents.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/move-shared-pointer-contents.cpp @@ -123,3 +123,16 @@ void rawPointer() { T* p; T x = std::move(*p); } + +struct HasASharedPtrField { + std::shared_ptr<Nontrivial> x; +}; + +HasASharedPtrField returnsStruct() { + HasASharedPtrField h; + return h; +} + +void subfield() { + int x = std::move(returnsStruct().x->x); +} >From 5cd19841fdef73950827220d06f78efe271f7008 Mon Sep 17 00:00:00 2001 From: David Pizzuto <piz...@google.com> Date: Mon, 1 Apr 2024 13:18:08 -0700 Subject: [PATCH 6/6] This check detects moves of the contents of a shared pointer rather than the pointer itself. Other code with a reference to the shared pointer is probably not expecting the move. The set of shared pointer classes is configurable via options to allow individual projects to cover additional types. --- .../MoveSharedPointerContentsCheck.cpp | 164 ++++++++---------- .../bugprone/MoveSharedPointerContentsCheck.h | 3 - clang-tools-extra/docs/ReleaseNotes.rst | 5 +- .../bugprone/move-shared-pointer-contents.rst | 15 +- .../bugprone/move-shared-pointer-contents.cpp | 26 +++ 5 files changed, 114 insertions(+), 99 deletions(-) diff --git a/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.cpp index 461fe91267e63..eb420a5708307 100644 --- a/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.cpp @@ -6,6 +6,9 @@ // //===----------------------------------------------------------------------===// +#include <signal.h> +#include <unistd.h> + #include "MoveSharedPointerContentsCheck.h" #include "../ClangTidyCheck.h" #include "../utils/Matchers.h" @@ -16,126 +19,111 @@ using namespace clang::ast_matchers; namespace clang::tidy::bugprone { +namespace { + +// Reports whether the QualType matches the inner matcher, which is expected to be +// matchesAnyListedName. The QualType is expected to either point to a RecordDecl +// (for concrete types) or an ElaboratedType (for dependent ones). +AST_MATCHER_P(QualType, isSharedPointer, + clang::ast_matchers::internal::Matcher<NamedDecl>, InnerMatcher) { + if (const auto *RD = Node.getTypePtr()->getAsCXXRecordDecl(); RD != nullptr) { + return InnerMatcher.matches(*RD, Finder, Builder); + } else if (const auto *ED = Node.getTypePtr()->getAs<ElaboratedType>(); + ED != nullptr) { + if (const auto *TS = ED->getNamedType() + .getTypePtr() + ->getAs<TemplateSpecializationType>(); + TS != nullptr) { + return InnerMatcher.matches(*TS->getTemplateName().getAsTemplateDecl(), + Finder, Builder); + } + } + + return false; +} + +} // namespace MoveSharedPointerContentsCheck::MoveSharedPointerContentsCheck( StringRef Name, ClangTidyContext *Context) : ClangTidyCheck(Name, Context), SharedPointerClasses(utils::options::parseStringList( - Options.get("SharedPointerClasses", "::std::shared_ptr"))) {} + Options.get("SharedPointerClasses", "::std::shared_ptr;::boost::shared_pointer"))) {} void MoveSharedPointerContentsCheck::registerMatchers(MatchFinder *Finder) { - auto isStdMove = callee(functionDecl(hasName("::std::move"))); - - // Resolved type, direct move. - Finder->addMatcher( - callExpr(isStdMove, hasArgument(0, cxxOperatorCallExpr( - hasOverloadedOperatorName("*"), - callee(cxxMethodDecl(ofClass( - matchers::matchesAnyListedName( - SharedPointerClasses))))))) - .bind("call"), - this); + auto isStdMove = callee(functionDecl(hasAnyName("::std::move", "::std::forward"))); - // Resolved type, move out of get(). - Finder->addMatcher( + auto resolvedType = callExpr(anyOf( + // Resolved type, direct move. + callExpr( + isStdMove, + hasArgument( + 0, + cxxOperatorCallExpr( + hasOverloadedOperatorName("*"), + hasDescendant(declRefExpr(hasType(qualType(isSharedPointer( + matchers::matchesAnyListedName(SharedPointerClasses)))))), + callee(cxxMethodDecl())))) + .bind("call"), + // Resolved type, move out of get(). callExpr( isStdMove, hasArgument( 0, unaryOperator( hasOperatorName("*"), - hasUnaryOperand(cxxMemberCallExpr(callee(cxxMethodDecl( - hasName("get"), ofClass(matchers::matchesAnyListedName( - SharedPointerClasses))))))))) - .bind("get_call"), - this); + hasUnaryOperand(allOf( + hasDescendant(declRefExpr(hasType(qualType( + isSharedPointer(matchers::matchesAnyListedName( + SharedPointerClasses)))))), + cxxMemberCallExpr( + callee(cxxMethodDecl(hasName("get"))))))))) + .bind("get_call"))); + + Finder->addMatcher(resolvedType, this); auto isStdMoveUnresolved = callee(unresolvedLookupExpr( hasAnyDeclaration(namedDecl(hasUnderlyingDecl(hasName("::std::move")))))); - // Unresolved type, direct move. - Finder->addMatcher( + auto unresolvedType = callExpr(anyOf( + // Unresolved type, direct move. callExpr( isStdMoveUnresolved, - hasArgument(0, unaryOperator(hasOperatorName("*"), - hasUnaryOperand(declRefExpr(hasType( - qualType().bind("unresolved_p"))))))) + hasArgument(0, unaryOperator( + hasOperatorName("*"), + hasUnaryOperand(declRefExpr(hasType(qualType( + isSharedPointer(matchers::matchesAnyListedName( + SharedPointerClasses))))))))) .bind("unresolved_call"), - this); - // Annoyingly, the declRefExpr in the unresolved-move-of-get() case - // is of <dependent type> rather than shared_ptr<T>, so we have to - // just fetch the variable. This does leave a gap where a temporary - // shared_ptr wouldn't be caught, but moving out of a temporary - // shared pointer is a truly wild thing to do so it should be okay. - - // Unresolved type, move out of get(). - Finder->addMatcher( + + // Annoyingly, the declRefExpr in the unresolved-move-of-get() case + // is of <dependent type> rather than shared_ptr<T>, so we have to + // just fetch the variable. This does leave a gap where a temporary + // shared_ptr wouldn't be caught, but moving out of a temporary + // shared pointer is a truly wild thing to do so it should be okay. callExpr(isStdMoveUnresolved, hasArgument( - 0, unaryOperator(hasOperatorName("*"), - hasDescendant(cxxDependentScopeMemberExpr( - hasMemberName("get"))), - hasDescendant(declRefExpr(to( - varDecl().bind("unresolved_get_p"))))))) - .bind("unresolved_get_call"), - this); -} - -bool MoveSharedPointerContentsCheck::isSharedPointerClass( - const VarDecl *VD) const { - if (VD == nullptr) - return false; - - const QualType QT = VD->getType(); - return isSharedPointerClass(&QT); -} - -bool MoveSharedPointerContentsCheck::isSharedPointerClass( - const QualType *QT) const { - if (QT == nullptr) - return false; - - // We want the qualified name without template parameters, - // const/volatile, or reference/pointer qualifiers so we can look - // it up in SharedPointerClasses. This is a bit messy, but gets us - // to the underlying type without template parameters (eg - // std::shared_ptr) or const/volatile qualifiers even in the face of - // typedefs. - - bool found = false; - const auto *Template = llvm::dyn_cast<TemplateSpecializationType>( - QT->getSplitDesugaredType().Ty); - if (Template != nullptr) { - const std::string TypeName = Template->getTemplateName() - .getAsTemplateDecl() - ->getQualifiedNameAsString(); - for (const llvm::StringRef SharedPointer : SharedPointerClasses) { - // SharedPointer entries may or may not have leading ::, but TypeName - // definitely won't. - if (SharedPointer == TypeName || SharedPointer.substr(2) == TypeName) { - found = true; - break; - } - } - } - - return found; + 0, unaryOperator( + hasOperatorName("*"), + hasDescendant(cxxDependentScopeMemberExpr( + hasMemberName("get"))), + hasDescendant(declRefExpr(to(varDecl(hasType(qualType( + isSharedPointer(matchers::matchesAnyListedName( + SharedPointerClasses))))))))))) + .bind("unresolved_get_call"))); + + Finder->addMatcher(unresolvedType, this); } void MoveSharedPointerContentsCheck::check( const MatchFinder::MatchResult &Result) { - const bool Unresolved = - isSharedPointerClass(Result.Nodes.getNodeAs<QualType>("unresolved_p")); - const bool UnresolvedGet = - isSharedPointerClass(Result.Nodes.getNodeAs<VarDecl>("unresolved_get_p")); - clang::SourceLocation Loc; if (const auto *UnresolvedCall = Result.Nodes.getNodeAs<CallExpr>("unresolved_call"); - UnresolvedCall != nullptr && Unresolved) { + UnresolvedCall != nullptr) { Loc = UnresolvedCall->getBeginLoc(); } else if (const auto *UnresolvedGetCall = Result.Nodes.getNodeAs<CallExpr>("unresolved_get_call"); - UnresolvedGetCall != nullptr && UnresolvedGet) { + UnresolvedGetCall != nullptr) { Loc = UnresolvedGetCall->getBeginLoc(); } else if (const auto *GetCall = Result.Nodes.getNodeAs<CallExpr>("get_call"); GetCall != nullptr) { diff --git a/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.h b/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.h index c3a6ca83b24ec..d2a42c49ddf67 100644 --- a/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.h +++ b/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.h @@ -34,9 +34,6 @@ class MoveSharedPointerContentsCheck : public ClangTidyCheck { } private: - // Returns whether the type or variable is one of the SharedPointerClasses. - bool isSharedPointerClass(const VarDecl *VD) const; - bool isSharedPointerClass(const QualType *QT) const; const std::vector<StringRef> SharedPointerClasses; }; diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 1238ef8336239..9f5bb39f10433 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -374,7 +374,8 @@ Changes in existing checks <clang-tidy/checks/misc/const-correctness>` check to avoid false positive when using pointer to member function. Additionally, the check no longer emits a diagnostic when a variable that is not type-dependent is an operand of a - type-dependent binary operator. + type-dependent binary operator. Improved performance of the check through + optimizations. - Improved :doc:`misc-include-cleaner <clang-tidy/checks/misc/include-cleaner>` check by adding option @@ -388,7 +389,7 @@ Changes in existing checks - Improved :doc:`misc-unused-using-decls <clang-tidy/checks/misc/unused-using-decls>` check to avoid false positive when - using in elaborated type. + using in elaborated type and only check cpp files. - Improved :doc:`modernize-avoid-bind <clang-tidy/checks/modernize/avoid-bind>` check to diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/move-shared-pointer-contents.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/move-shared-pointer-contents.rst index ea4c1fb6df708..03104a389d119 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/bugprone/move-shared-pointer-contents.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/move-shared-pointer-contents.rst @@ -6,14 +6,17 @@ bugprone-move-shared-pointer-contents Detects calls to move the contents out of a ``std::shared_ptr`` rather than moving the pointer itself. In other words, calling -``std::move(*p)`` or ``std::move(*p.get())``. Other reference holders -may not be expecting the move and suddenly getting empty or otherwise -indeterminate states can cause issues. +``std::move(*p)`` or ``std::move(*p.get())`` or similar calls with +``std::forward``. Other reference holders may not be expecting the +move and suddenly getting empty or otherwise indeterminate states can +cause issues. Only applies to C++11 and above, as that's when +``std::shared_ptr`` was introduced. Options ------- .. option :: SharedPointerClasses - A semicolon-separated list of class names that should be treated as shared - pointers. Classes are resolved through aliases, so any alias to the defined - classes will be considered. Default is `::std::shared_ptr`. + A semicolon-separated list of class names that should be treated as + shared pointers. Classes are resolved through aliases, so any alias + to the defined classes will be considered. Default is + `::std::shared_ptr` and `::boost::shared_pointer`. diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/move-shared-pointer-contents.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/move-shared-pointer-contents.cpp index ccc4400aa691c..00fa58635c6ee 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/move-shared-pointer-contents.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/move-shared-pointer-contents.cpp @@ -16,6 +16,11 @@ constexpr typename std::remove_reference<_Tp>::type &&move(_Tp &&__t) { return static_cast<typename std::remove_reference<_Tp>::type &&>(__t); } +template <typename _Tp> +constexpr _Tp&& forward(typename std::remove_reference<_Tp>::type &__t) { + return static_cast<_Tp>(__t); +} + template <typename T> struct shared_ptr { shared_ptr(); @@ -45,6 +50,8 @@ struct Nontrivial { Nontrivial& operator=(Nontrivial&& other) { x = std::move(other.x); } }; +void target(Nontrivial&& n) {} + // Test cases begin here. void correct() { @@ -58,6 +65,12 @@ void simpleFinding() { } // CHECK-MESSAGES: :[[@LINE-2]]:18: warning: don't move the contents out of a shared pointer, as other accessors expect them to remain in a determinate state [bugprone-move-shared-pointer-contents] +void simpleForwardFinding() { + std::shared_ptr<Nontrivial> p; + target(std::forward<Nontrivial>(*p)); +} +// CHECK-MESSAGES: :[[@LINE-2]]:10: warning: don't move the contents out of a shared pointer, as other accessors expect them to remain in a determinate state [bugprone-move-shared-pointer-contents] + void aliasedType() { using nontrivial_ptr = std::shared_ptr<Nontrivial>; nontrivial_ptr p; @@ -123,3 +136,16 @@ void rawPointer() { T* p; T x = std::move(*p); } + +struct HasASharedPtrField { + std::shared_ptr<Nontrivial> x; +}; + +HasASharedPtrField returnsStruct() { + HasASharedPtrField h; + return h; +} + +void subfield() { + int x = std::move(returnsStruct().x->x); +} _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
