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 01/12] [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 02/12] [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 03/12] [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 04/12] [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 05/12] 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 06/12] 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); +} >From 131b99d928ee061b85b4872e108f0bb73bf042d5 Mon Sep 17 00:00:00 2001 From: David Pizzuto <piz...@google.com> Date: Fri, 26 Jul 2024 15:22:55 -0700 Subject: [PATCH 07/12] merge --- .../MoveSharedPointerContentsCheck.cpp | 3 - clang-tools-extra/docs/ReleaseNotes.rst | 379 +----------------- .../docs/clang-tidy/checks/list.rst | 2 +- .../bugprone/move-shared-pointer-contents.cpp | 14 +- .../modernize/Inputs/smart-ptr/shared_ptr.h | 1 + 5 files changed, 11 insertions(+), 388 deletions(-) diff --git a/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.cpp index eb420a5708307..57f3eadf91139 100644 --- a/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.cpp @@ -6,9 +6,6 @@ // //===----------------------------------------------------------------------===// -#include <signal.h> -#include <unistd.h> - #include "MoveSharedPointerContentsCheck.h" #include "../ClangTidyCheck.h" #include "../utils/Matchers.h" diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 1f0db8762bd3d..9aed2cbda1354 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -69,9 +69,6 @@ Code completion Code actions ^^^^^^^^^^^^ -- The tweak for turning unscoped into scoped enums now removes redundant prefixes - from the enum values. - Signature help ^^^^^^^^^^^^^^ @@ -84,23 +81,12 @@ Objective-C Miscellaneous ^^^^^^^^^^^^^ -- Added a boolean option `AnalyzeAngledIncludes` to `Includes` config section, - which allows to enable unused includes detection for all angled ("system") headers. - At this moment umbrella headers are not supported, so enabling this option - may result in false-positives. - Improvements to clang-doc ------------------------- Improvements to clang-query --------------------------- -- Added the `file` command to dynamically load a list of commands and matchers - from an external file, allowing the cost of reading the compilation database - and building the AST to be imposed just once for faster prototyping. - -- Removed support for ``enable output srcloc``. Fixes #GH82591 - Improvements to clang-rename ---------------------------- @@ -109,383 +95,30 @@ The improvements are... Improvements to clang-tidy -------------------------- -- Improved :program:`run-clang-tidy.py` script. Added argument `-source-filter` - to filter source files from the compilation database, via a RegEx. In a - similar fashion to what `-header-filter` does for header files. - -- Improved :program:`check_clang_tidy.py` script. Added argument `-export-fixes` - to aid in clang-tidy and test development. - -- Fixed bug where big values for unsigned check options overflowed into negative values - when being printed with `--dump-config`. - -- Fixed `--verify-config` option not properly parsing checks when using the - literal operator in the `.clang-tidy` config. - -- Added argument `--exclude-header-filter` and config option `ExcludeHeaderFilterRegex` - to exclude headers from analysis via a RegEx. - New checks ^^^^^^^^^^ -- New :doc:`bugprone-crtp-constructor-accessibility - <clang-tidy/checks/bugprone/crtp-constructor-accessibility>` check. - - Detects error-prone Curiously Recurring Template Pattern usage, when the CRTP - can be constructed outside itself and the derived class. - -- New :doc:`bugprone-return-const-ref-from-parameter - <clang-tidy/checks/bugprone/return-const-ref-from-parameter>` check. - - Detects return statements that return a constant reference parameter as constant - reference. This may cause use-after-free errors if the caller uses xvalues as - arguments. - -- New :doc:`bugprone-suspicious-stringview-data-usage - <clang-tidy/checks/bugprone/suspicious-stringview-data-usage>` check. - - Identifies suspicious usages of ``std::string_view::data()`` that could lead - to reading out-of-bounds data due to inadequate or incorrect string null - termination. - -- New :doc:`misc-use-internal-linkage - <clang-tidy/checks/misc/use-internal-linkage>` check. - - Detects variables and functions that can be marked as static or moved into - an anonymous namespace to enforce internal linkage. - -- 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. - -- New :doc:`modernize-min-max-use-initializer-list - <clang-tidy/checks/modernize/min-max-use-initializer-list>` check. - - Replaces nested ``std::min`` and ``std::max`` calls with an initializer list - where applicable. - -- New :doc:`modernize-use-designated-initializers - <clang-tidy/checks/modernize/use-designated-initializers>` check. - - Finds initializer lists for aggregate types that could be - written as designated initializers instead. - -- New :doc:`modernize-use-std-format - <clang-tidy/checks/modernize/use-std-format>` check. - - Converts calls to ``absl::StrFormat``, or other functions via - configuration options, to C++20's ``std::format``, or another function - via a configuration option, modifying the format string appropriately and - removing now-unnecessary calls to ``std::string::c_str()`` and - ``std::string::data()``. - -- New :doc:`readability-enum-initial-value - <clang-tidy/checks/readability/enum-initial-value>` check. - - Enforces consistent style for enumerators' initialization, covering three - styles: none, first only, or all initialized explicitly. - -- New :doc:`readability-math-missing-parentheses - <clang-tidy/checks/readability/math-missing-parentheses>` check. - - Check for missing parentheses in mathematical expressions that involve - operators of different priorities. +New check aliases +^^^^^^^^^^^^^^^^^ -- New :doc:`readability-use-std-min-max - <clang-tidy/checks/readability/use-std-min-max>` check. +Changes in existing checks +^^^^^^^^^^^^^^^^^^^^^^^^^^ - Replaces certain conditional statements with equivalent calls to - ``std::min`` or ``std::max``. +New checks +^^^^^^^^^^ New check aliases ^^^^^^^^^^^^^^^^^ -- New alias :doc:`cert-int09-c <clang-tidy/checks/cert/int09-c>` to - :doc:`readability-enum-initial-value <clang-tidy/checks/readability/enum-initial-value>` - was added. - Changes in existing checks ^^^^^^^^^^^^^^^^^^^^^^^^^^ -- Improved :doc:`bugprone-assert-side-effect - <clang-tidy/checks/bugprone/assert-side-effect>` check by detecting side - effect from calling a method with non-const reference parameters. - -- Improved :doc:`bugprone-casting-through-void - <clang-tidy/checks/bugprone/casting-through-void>` check by ignoring casts - where source is already a ``void``` pointer, making middle ``void`` pointer - casts bug-free. - -- Improved :doc:`bugprone-forwarding-reference-overload - <clang-tidy/checks/bugprone/forwarding-reference-overload>` - check to ignore deleted constructors which won't hide other overloads. - -- Improved :doc:`bugprone-inc-dec-in-conditions - <clang-tidy/checks/bugprone/inc-dec-in-conditions>` check to ignore code - within unevaluated contexts, such as ``decltype``. - -- Improved :doc:`bugprone-lambda-function-name<clang-tidy/checks/bugprone/lambda-function-name>` - check by ignoring ``__func__`` macro in lambda captures, initializers of - default parameters and nested function declarations. - -- Improved :doc:`bugprone-multi-level-implicit-pointer-conversion - <clang-tidy/checks/bugprone/multi-level-implicit-pointer-conversion>` check - by ignoring implicit pointer conversions that are part of a cast expression. - -- Improved :doc:`bugprone-non-zero-enum-to-bool-conversion - <clang-tidy/checks/bugprone/non-zero-enum-to-bool-conversion>` check by - eliminating false positives resulting from direct usage of bitwise operators - within parentheses. - -- Improved :doc:`bugprone-optional-value-conversion - <clang-tidy/checks/bugprone/optional-value-conversion>` check by eliminating - false positives resulting from use of optionals in unevaluated context. - -- Improved :doc:`bugprone-sizeof-expression - <clang-tidy/checks/bugprone/sizeof-expression>` check by eliminating some - false positives and adding a new (off-by-default) option - `WarnOnSizeOfPointer` that reports all ``sizeof(pointer)`` expressions - (except for a few that are idiomatic). - -- Improved :doc:`bugprone-suspicious-include - <clang-tidy/checks/bugprone/suspicious-include>` check by replacing the local - options `HeaderFileExtensions` and `ImplementationFileExtensions` by the - global options of the same name. - -- Improved :doc:`bugprone-too-small-loop-variable - <clang-tidy/checks/bugprone/too-small-loop-variable>` check by incorporating - better support for ``const`` loop boundaries. - -- Improved :doc:`bugprone-unused-local-non-trivial-variable - <clang-tidy/checks/bugprone/unused-local-non-trivial-variable>` check by - ignoring local variable with ``[maybe_unused]`` attribute. - -- Improved :doc:`bugprone-unused-return-value - <clang-tidy/checks/bugprone/unused-return-value>` check by updating the - parameter `CheckedFunctions` to support regexp, avoiding false positive for - function with the same prefix as the default argument, e.g. ``std::unique_ptr`` - and ``std::unique``, avoiding false positive for assignment operator overloading. - -- Improved :doc:`bugprone-use-after-move - <clang-tidy/checks/bugprone/use-after-move>` check to also handle - calls to ``std::forward``. - -- Improved :doc:`cppcoreguidelines-macro-usage - <clang-tidy/checks/cppcoreguidelines/macro-usage>` check by ignoring macro with - hash preprocessing token. - -- Improved :doc:`cppcoreguidelines-missing-std-forward - <clang-tidy/checks/cppcoreguidelines/missing-std-forward>` check by no longer - giving false positives for deleted functions, by fixing false negatives when only - a few parameters are forwarded and by ignoring parameters without a name (unused - arguments). - -- Improved :doc:`cppcoreguidelines-owning-memory - <clang-tidy/checks/cppcoreguidelines/owning-memory>` check to properly handle - return type in lambdas and in nested functions. - -- Improved :doc:`cppcoreguidelines-prefer-member-initializer - <clang-tidy/checks/cppcoreguidelines/prefer-member-initializer>` check - by removing enforcement of rule `C.48 - <https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c48-prefer-in-class-initializers-to-member-initializers-in-constructors-for-constant-initializers>`_, - which was deprecated since :program:`clang-tidy` 17. This rule is now covered - by :doc:`cppcoreguidelines-use-default-member-init - <clang-tidy/checks/cppcoreguidelines/use-default-member-init>`. Fixed - incorrect hints when using list-initialization. - -- Improved :doc:`cppcoreguidelines-special-member-functions - <clang-tidy/checks/cppcoreguidelines/special-member-functions>` check with a - new option `AllowImplicitlyDeletedCopyOrMove`, which removes the requirement - for explicit copy or move special member functions when they are already - implicitly deleted. - -- Improved :doc:`google-build-namespaces - <clang-tidy/checks/google/build-namespaces>` check by replacing the local - option `HeaderFileExtensions` by the global option of the same name. - -- Improved :doc:`google-explicit-constructor - <clang-tidy/checks/google/explicit-constructor>` check to better handle - C++20 `explicit(bool)`. - -- Improved :doc:`google-global-names-in-headers - <clang-tidy/checks/google/global-names-in-headers>` check by replacing the local - option `HeaderFileExtensions` by the global option of the same name. - -- Improved :doc:`google-runtime-int <clang-tidy/checks/google/runtime-int>` - check performance through optimizations. - -- Improved :doc:`hicpp-signed-bitwise <clang-tidy/checks/hicpp/signed-bitwise>` - check by ignoring false positives involving positive integer literals behind - implicit casts when `IgnorePositiveIntegerLiterals` is enabled. - -- Improved :doc:`hicpp-ignored-remove-result <clang-tidy/checks/hicpp/ignored-remove-result>` - check by ignoring other functions with same prefixes as the target specific - functions. - -- Improved :doc:`linuxkernel-must-check-errs - <clang-tidy/checks/linuxkernel/must-check-errs>` check documentation to - consistently use the check's proper name. - -- Improved :doc:`llvm-header-guard - <clang-tidy/checks/llvm/header-guard>` check by replacing the local - option `HeaderFileExtensions` by the global option of the same name. - -- Improved :doc:`misc-const-correctness - <clang-tidy/checks/misc/const-correctness>` check by avoiding infinite recursion - for recursive functions with forwarding reference parameters and reference - variables which refer to themselves. - -- Improved :doc:`misc-definitions-in-headers - <clang-tidy/checks/misc/definitions-in-headers>` check by replacing the local - option `HeaderFileExtensions` by the global option of the same name. - Additionally, the option `UseHeaderFileExtensions` is removed, so that the - check uses the `HeaderFileExtensions` option unconditionally. - -- Improved :doc:`misc-header-include-cycle - <clang-tidy/checks/misc/header-include-cycle>` check by avoiding crash for self - include cycles. - -- Improved :doc:`misc-unused-using-decls - <clang-tidy/checks/misc/unused-using-decls>` check by replacing the local - option `HeaderFileExtensions` by the global option of the same name. - -- Improved :doc:`misc-use-anonymous-namespace - <clang-tidy/checks/misc/use-anonymous-namespace>` check by replacing the local - option `HeaderFileExtensions` by the global option of the same name. - -- Improved :doc:`modernize-avoid-c-arrays - <clang-tidy/checks/modernize/avoid-c-arrays>` check by introducing the new - `AllowStringArrays` option, enabling the exclusion of array types with deduced - length initialized from string literals. - -- Improved :doc:`modernize-loop-convert - <clang-tidy/checks/modernize/loop-convert>` check by ensuring that fix-its - don't remove parentheses used in ``sizeof`` calls when they have array index - accesses as arguments. - -- Improved :doc:`modernize-use-constraints - <clang-tidy/checks/modernize/use-constraints>` check by fixing a crash that - occurred in some scenarios and excluding system headers from analysis. - -- Improved :doc:`modernize-use-nullptr - <clang-tidy/checks/modernize/use-nullptr>` check to include support for C23, - which also has introduced the ``nullptr`` keyword. - -- Improved :doc:`modernize-use-override - <clang-tidy/checks/modernize/use-override>` check to also remove any trailing - whitespace when deleting the ``virtual`` keyword. - -- Improved :doc:`modernize-use-starts-ends-with - <clang-tidy/checks/modernize/use-starts-ends-with>` check to also handle - calls to ``compare`` method. - -- Improved :doc:`modernize-use-std-print - <clang-tidy/checks/modernize/use-std-print>` check to not crash if the - format string parameter of the function to be replaced is not of the - expected type. - -- Improved :doc:`modernize-use-using <clang-tidy/checks/modernize/use-using>` - check by adding support for detection of typedefs declared on function level. - -- Improved :doc:`performance-inefficient-vector-operation - <clang-tidy/checks/performance/inefficient-vector-operation>` fixing false - negatives caused by different variable definition type and variable initial - value type in loop initialization expression. - -- Improved :doc:`performance-move-const-arg - <clang-tidy/checks/performance/move-const-arg>` check by ignoring - ``std::move()`` calls when their target is used as an rvalue. - -- Improved :doc:`performance-unnecessary-copy-initialization - <clang-tidy/checks/performance/unnecessary-copy-initialization>` check by - detecting more cases of constant access. In particular, pointers can be - analyzed, so the check now handles the common patterns - `const auto e = (*vector_ptr)[i]` and `const auto e = vector_ptr->at(i);`. - Calls to mutable function where there exists a `const` overload are also - handled. - -- Improved :doc:`readability-avoid-return-with-void-value - <clang-tidy/checks/readability/avoid-return-with-void-value>` check by adding - fix-its. - -- Improved :doc:`readability-const-return-type - <clang-tidy/checks/readability/const-return-type>` check to eliminate false - positives when returning types with const not at the top level. - -- Improved :doc:`readability-container-size-empty - <clang-tidy/checks/readability/container-size-empty>` check to prevent false - positives when utilizing ``size`` or ``length`` methods that accept parameter. - Fixed crash when facing template user defined literals. - -- Improved :doc:`readability-duplicate-include - <clang-tidy/checks/readability/duplicate-include>` check by excluding include - directives that form the filename using macro. - -- Improved :doc:`readability-else-after-return - <clang-tidy/checks/readability/else-after-return>` check to ignore - `if consteval` statements, for which the `else` branch must not be removed. - -- Improved :doc:`readability-identifier-naming - <clang-tidy/checks/readability/identifier-naming>` check in `GetConfigPerFile` - mode by resolving symbolic links to header files. Fixed handling of Hungarian - Prefix when configured to `LowerCase`. Added support for renaming designated - initializers. Added support for renaming macro arguments. Fixed renaming - conflicts arising from out-of-line member function template definitions. - -- Improved :doc:`readability-implicit-bool-conversion - <clang-tidy/checks/readability/implicit-bool-conversion>` check to provide - valid fix suggestions for ``static_cast`` without a preceding space and - fixed problem with duplicate parentheses in double implicit casts. Corrected - the fix suggestions for C23 and later by using C-style casts instead of - ``static_cast``. Fixed false positives in C++20 spaceship operator by ignoring - casts in implicit and defaulted functions. - -- Improved :doc:`readability-redundant-inline-specifier - <clang-tidy/checks/readability/redundant-inline-specifier>` check to properly - emit warnings for static data member with an in-class initializer. - -- Improved :doc:`readability-redundant-member-init - <clang-tidy/checks/readability/redundant-member-init>` check to avoid - false-positives when type of the member does not match the type of the - initializer. - -- Improved :doc:`readability-static-accessed-through-instance - <clang-tidy/checks/readability/static-accessed-through-instance>` check to - support calls to overloaded operators as base expression and provide fixes to - expressions with side-effects. - -- Improved :doc:`readability-simplify-boolean-expr - <clang-tidy/checks/readability/simplify-boolean-expr>` check to avoid to emit - warning for macro when IgnoreMacro option is enabled. - -- Improved :doc:`readability-static-definition-in-anonymous-namespace - <clang-tidy/checks/readability/static-definition-in-anonymous-namespace>` - check by resolving fix-it overlaps in template code by disregarding implicit - instances. - -- Improved :doc:`readability-string-compare - <clang-tidy/checks/readability/string-compare>` check to also detect - usages of ``std::string_view::compare``. Added a `StringLikeClasses` option - to detect usages of ``compare`` method in custom string-like classes. - Removed checks ^^^^^^^^^^^^^^ -- Removed `cert-dcl21-cpp`, which was deprecated since :program:`clang-tidy` 17, - since the rule DCL21-CPP has been removed from the CERT guidelines. - Miscellaneous ^^^^^^^^^^^^^ -- Fixed incorrect formatting in :program:`clang-apply-replacements` when no - `--format` option is specified. Now :program:`clang-apply-replacements` - applies formatting only with the option. - Improvements to include-fixer ----------------------------- diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index c0e164f4996c9..99969c088da56 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -119,7 +119,7 @@ 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", + :doc:`bugprone-reserved-identifier <bugprone/reserved-identifier>`, "Yes" :doc:`bugprone-reserved-identifier <bugprone/reserved-identifier>`, "Yes" :doc:`bugprone-return-const-ref-from-parameter <bugprone/return-const-ref-from-parameter>`, :doc:`bugprone-shared-ptr-array-mismatch <bugprone/shared-ptr-array-mismatch>`, "Yes" 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 00fa58635c6ee..806e29c4ddc19 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 @@ -1,7 +1,9 @@ -// RUN: %check_clang_tidy %s bugprone-move-shared-pointer-contents %t -- -config="{CheckOptions: {bugprone-move-shared-pointer-contents.SharedPointerClasses: '::std::shared_ptr;my::OtherSharedPtr;'}}" +// RUN: %check_clang_tidy %s bugprone-move-shared-pointer-contents %t -- -config="{CheckOptions: {bugprone-move-shared-pointer-contents.SharedPointerClasses: '::std::shared_ptr;my::OtherSharedPtr;'}}" -- -I %S/../modernize/Inputs/smart-ptr // Some dummy definitions we'll need. +#include "shared_ptr.h" + namespace std { using size_t = int; @@ -21,16 +23,6 @@ constexpr _Tp&& forward(typename std::remove_reference<_Tp>::type &__t) { return static_cast<_Tp>(__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 { diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/Inputs/smart-ptr/shared_ptr.h b/clang-tools-extra/test/clang-tidy/checkers/modernize/Inputs/smart-ptr/shared_ptr.h index 337cb28228b09..dbd25e1e3350e 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/modernize/Inputs/smart-ptr/shared_ptr.h +++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/Inputs/smart-ptr/shared_ptr.h @@ -10,6 +10,7 @@ class __shared_ptr { type &operator*() { return *ptr; } type *operator->() { return ptr; } type *release(); + type *get() const; void reset(); void reset(type *pt); >From 52d93536b32df9f07dd8dfe60bd774a7e4e76dba Mon Sep 17 00:00:00 2001 From: David Pizzuto <piz...@google.com> Date: Fri, 26 Jul 2024 16:06:04 -0700 Subject: [PATCH 08/12] cleanup duplicate entry --- clang-tools-extra/docs/clang-tidy/checks/list.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index 99969c088da56..c9aa10a1f3bdd 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -109,6 +109,7 @@ Clang-Tidy Checks :doc:`bugprone-misplaced-pointer-arithmetic-in-alloc <bugprone/misplaced-pointer-arithmetic-in-alloc>`, "Yes" :doc:`bugprone-misplaced-widening-cast <bugprone/misplaced-widening-cast>`, :doc:`bugprone-move-forwarding-reference <bugprone/move-forwarding-reference>`, "Yes" + :doc:`bugprone-move-shared-pointer-contents <bugprone/move-shared-pointer-contents>`, "Yes" :doc:`bugprone-multi-level-implicit-pointer-conversion <bugprone/multi-level-implicit-pointer-conversion>`, :doc:`bugprone-multiple-new-in-one-expression <bugprone/multiple-new-in-one-expression>`, :doc:`bugprone-multiple-statement-macro <bugprone/multiple-statement-macro>`, @@ -120,7 +121,6 @@ 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" - :doc:`bugprone-reserved-identifier <bugprone/reserved-identifier>`, "Yes" :doc:`bugprone-return-const-ref-from-parameter <bugprone/return-const-ref-from-parameter>`, :doc:`bugprone-shared-ptr-array-mismatch <bugprone/shared-ptr-array-mismatch>`, "Yes" :doc:`bugprone-shared-pointer-contents-move <bugprone/move-shared-pointer-contents.rst>`, >From 83ffdd8ab447e2fe9432a527647b7c870f5c4818 Mon Sep 17 00:00:00 2001 From: David Pizzuto <piz...@google.com> Date: Fri, 26 Jul 2024 16:08:30 -0700 Subject: [PATCH 09/12] cleanup a dupe and restore release notes --- clang-tools-extra/docs/ReleaseNotes.rst | 6 ++++++ clang-tools-extra/docs/clang-tidy/checks/list.rst | 1 - 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 9aed2cbda1354..8455e9e7c5815 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -107,6 +107,12 @@ Changes in existing checks New checks ^^^^^^^^^^ +- 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 check aliases ^^^^^^^^^^^^^^^^^ diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index c9aa10a1f3bdd..a9a4e02197c48 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -123,7 +123,6 @@ Clang-Tidy Checks :doc:`bugprone-reserved-identifier <bugprone/reserved-identifier>`, "Yes" :doc:`bugprone-return-const-ref-from-parameter <bugprone/return-const-ref-from-parameter>`, :doc:`bugprone-shared-ptr-array-mismatch <bugprone/shared-ptr-array-mismatch>`, "Yes" - :doc:`bugprone-shared-pointer-contents-move <bugprone/move-shared-pointer-contents.rst>`, :doc:`bugprone-signal-handler <bugprone/signal-handler>`, :doc:`bugprone-signed-char-misuse <bugprone/signed-char-misuse>`, :doc:`bugprone-sizeof-container <bugprone/sizeof-container>`, >From 14028ab9165b643c498a375b56de31e841e488b7 Mon Sep 17 00:00:00 2001 From: David Pizzuto <piz...@google.com> Date: Fri, 26 Jul 2024 16:12:14 -0700 Subject: [PATCH 10/12] avoid merge conflict --- clang-tools-extra/docs/ReleaseNotes.rst | 9 --------- 1 file changed, 9 deletions(-) diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 8455e9e7c5815..25d38b959ad82 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -98,15 +98,6 @@ Improvements to clang-tidy New checks ^^^^^^^^^^ -New check aliases -^^^^^^^^^^^^^^^^^ - -Changes in existing checks -^^^^^^^^^^^^^^^^^^^^^^^^^^ - -New checks -^^^^^^^^^^ - - New :doc:`bugprone-move-shared-pointer-contents <clang-tidy/checks/bugprone/move-shared-pointer-contents>` check. >From 4e516c0583f96684746cc36d8f40e90bcf4ca6dc Mon Sep 17 00:00:00 2001 From: pizzud <123112817+piz...@users.noreply.github.com> Date: Fri, 26 Jul 2024 16:25:22 -0700 Subject: [PATCH 11/12] Address formatting suggestion in checker doc. Co-authored-by: EugeneZelenko <eugene.zele...@gmail.com> --- .../clang-tidy/checks/bugprone/move-shared-pointer-contents.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 03104a389d119..b1de44b79fc18 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 @@ -19,4 +19,4 @@ Options 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`. + `::std::shared_ptr;::boost::shared_pointer`. >From 70fdd1d9ccefe7c1dade13ba70d7878bd537223a Mon Sep 17 00:00:00 2001 From: David Pizzuto <piz...@google.com> Date: Fri, 9 Aug 2024 15:21:12 -0700 Subject: [PATCH 12/12] reflow check() implementation and add another unit test --- .../MoveSharedPointerContentsCheck.cpp | 55 +++++++++---------- .../bugprone/move-shared-pointer-contents.cpp | 7 +++ 2 files changed, 32 insertions(+), 30 deletions(-) diff --git a/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.cpp index 57f3eadf91139..4cade83469339 100644 --- a/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.cpp @@ -6,6 +6,8 @@ // //===----------------------------------------------------------------------===// +#include <iostream> + #include "MoveSharedPointerContentsCheck.h" #include "../ClangTidyCheck.h" #include "../utils/Matchers.h" @@ -52,15 +54,15 @@ void MoveSharedPointerContentsCheck::registerMatchers(MatchFinder *Finder) { auto resolvedType = callExpr(anyOf( // Resolved type, direct move. - callExpr( - isStdMove, - hasArgument( - 0, - cxxOperatorCallExpr( - hasOverloadedOperatorName("*"), - hasDescendant(declRefExpr(hasType(qualType(isSharedPointer( - matchers::matchesAnyListedName(SharedPointerClasses)))))), - callee(cxxMethodDecl())))) + callExpr(isStdMove, + hasArgument( + 0, cxxOperatorCallExpr( + hasOverloadedOperatorName("*"), + hasArgument( + 0, declRefExpr(hasType(qualType(isSharedPointer( + matchers::matchesAnyListedName( + SharedPointerClasses)))))), + callee(cxxMethodDecl())))) .bind("call"), // Resolved type, move out of get(). callExpr( @@ -113,30 +115,23 @@ void MoveSharedPointerContentsCheck::registerMatchers(MatchFinder *Finder) { void MoveSharedPointerContentsCheck::check( const MatchFinder::MatchResult &Result) { - clang::SourceLocation Loc; - if (const auto *UnresolvedCall = - Result.Nodes.getNodeAs<CallExpr>("unresolved_call"); - UnresolvedCall != nullptr) { - Loc = UnresolvedCall->getBeginLoc(); - } else if (const auto *UnresolvedGetCall = - Result.Nodes.getNodeAs<CallExpr>("unresolved_get_call"); - UnresolvedGetCall != nullptr) { - 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; + const CallExpr *Call = nullptr; + for (const llvm::StringRef binding : + {"unresolved_call", "unresolved_get_call", "get_call", "call"}) { + if (const auto *C = Result.Nodes.getNodeAs<CallExpr>(binding); + C != nullptr) { + Call = C; + break; + } } - 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"); + if (Call == nullptr && !Call->getBeginLoc().isValid()) { + return; } + + diag(Call->getBeginLoc(), + "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/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 806e29c4ddc19..be98f48a3a7f0 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 @@ -141,3 +141,10 @@ HasASharedPtrField returnsStruct() { void subfield() { int x = std::move(returnsStruct().x->x); } + +Nontrivial* dummy(int, std::shared_ptr<Nontrivial>) { return nullptr; } + +void sharedPointerIsOtherParameter() { + std::shared_ptr<Nontrivial> p; + Nontrivial n = std::move(*dummy(1, p)); +} _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
