https://github.com/pizzud updated https://github.com/llvm/llvm-project/pull/67467
>From 6d5d35e1273f595e8a0382053d5183cbce7a9d8a 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/5] [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 | 2 + .../MoveSharedPointerContentsCheck.cpp | 60 ++++++++++++++++ .../bugprone/MoveSharedPointerContentsCheck.h | 37 ++++++++++ clang-tools-extra/docs/ReleaseNotes.rst | 6 ++ .../bugprone/move-shared-pointer-contents.rst | 17 +++++ .../docs/clang-tidy/checks/list.rst | 2 + .../bugprone/move-shared-pointer-contents.cpp | 68 +++++++++++++++++++ 8 files changed, 195 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 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 a67a91eedd1048..7f4a504f9930f1 100644 --- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp @@ -39,6 +39,7 @@ #include "MisplacedPointerArithmeticInAllocCheck.h" #include "MisplacedWideningCastCheck.h" #include "MoveForwardingReferenceCheck.h" +#include "MoveSharedPointerContentsCheck.h" #include "MultiLevelImplicitPointerConversionCheck.h" #include "MultipleNewInOneExpressionCheck.h" #include "MultipleStatementMacroCheck.h" @@ -125,6 +126,8 @@ class BugproneModule : public ClangTidyModule { "bugprone-inaccurate-erase"); CheckFactories.registerCheck<IncorrectEnableIfCheck>( "bugprone-incorrect-enable-if"); + CheckFactories.registerCheck<MoveSharedPointerContentsCheck>( + "bugprone-move-shared-pointer-contents"); CheckFactories.registerCheck<SwitchMissingDefaultCaseCheck>( "bugprone-switch-missing-default-case"); CheckFactories.registerCheck<IncDecInConditionsCheck>( diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt index 3c768021feb150..c017f0c0cc5202 100644 --- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt @@ -23,6 +23,7 @@ add_clang_library(clangTidyBugproneModule ImplicitWideningOfMultiplicationResultCheck.cpp InaccurateEraseCheck.cpp IncorrectEnableIfCheck.cpp + MoveSharedPointerContentsCheck.cpp SwitchMissingDefaultCaseCheck.cpp IncDecInConditionsCheck.cpp IncorrectRoundingsCheck.cpp @@ -35,6 +36,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/clang-tidy/bugprone/MoveSharedPointerContentsCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.cpp new file mode 100644 index 00000000000000..b4a393b7f2f200 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.cpp @@ -0,0 +1,60 @@ +//===--- 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"))) {} + +MoveSharedPointerContentsCheck::~MoveSharedPointerContentsCheck() = default; + +bool MoveSharedPointerContentsCheck::isLanguageVersionSupported( + const LangOptions &LangOptions) const { + return LangOptions.CPlusPlus11; +} + +void MoveSharedPointerContentsCheck::registerMatchers(MatchFinder *Finder) { + Finder->addMatcher( + callExpr( + callee(functionDecl(hasName("std::move"))), + hasArgument(0, cxxOperatorCallExpr(hasOverloadedOperatorName("*"), + callee(cxxMethodDecl(ofClass( + matchers::matchesAnyListedName( + SharedPointerClasses))))) + .bind("op"))) + .bind("call"), + this); +} + +void MoveSharedPointerContentsCheck::check( + const MatchFinder::MatchResult &Result) { + const auto *Call = Result.Nodes.getNodeAs<CallExpr>("call"); + const auto *Op = Result.Nodes.getNodeAs<Expr>("op"); + + if (Op) { + diag(Call->getBeginLoc(), + "don't move the contents out of a shared pointer, as other accessors " + "expect them to remain in a determinate state") + << FixItHint::CreateInsertion(Call->getBeginLoc(), "*") + << FixItHint::CreateRemoval(Op->getBeginLoc()); + } +} + +} // 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 00000000000000..a9ae8d335941e8 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.h @@ -0,0 +1,37 @@ +//===--- 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 { + +/// FIXME: Write a short description. +/// +/// 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); + ~MoveSharedPointerContentsCheck() override; + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + bool + isLanguageVersionSupported(const LangOptions &LangOptions) const override; + +private: + 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/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 19c977977f9044..cbd472d86c4227 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -138,6 +138,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. 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 00000000000000..4536663a1b7470 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/move-shared-pointer-contents.rst @@ -0,0 +1,17 @@ +.. 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)``. 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. By default only `std::shared_ptr` is included. diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index 75a86431d26441..bfe8b15ccc7ea3 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -105,6 +105,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>`, @@ -116,6 +117,7 @@ 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-shared-pointer-contents-move <bugprone/shared-pointer-contents-move>`, "Yes" :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>`, 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 00000000000000..0837708a7171c5 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/move-shared-pointer-contents.cpp @@ -0,0 +1,68 @@ +// 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>; +} // namespace my + +void correct() { + std::shared_ptr<int> p; + int x = *std::move(p); +} + +void simpleFinding() { + std::shared_ptr<int> p; + int y = std::move(*p); + // CHECK-FIXES: *std::move(p) +} +// CHECK-MESSAGES: :[[@LINE-3]]:11: 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 int_ptr = std::shared_ptr<int>; + int_ptr p; + int x = std::move(*p); + // CHECK-FIXES: *std::move(p) +} +// CHECK-MESSAGES: :[[@LINE-3]]:11: 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<int> p; + int x = std::move(*p); + // CHECK-FIXES: *std::move(p) +} +// CHECK-MESSAGES: :[[@LINE-3]]:11: 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 multiStars() { + std::shared_ptr<int> p; + int x = 2 * std::move(*p) * 3; + // CHECK-FIXES: *std::move(p) +} +// CHECK-MESSAGES: :[[@LINE-3]]: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] >From 528146e8d1fae56d2d4cc2d32a0823aa1f9cbc2b 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/5] [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 | 19 +++++-------------- .../bugprone/MoveSharedPointerContentsCheck.h | 10 ++++++---- .../bugprone/move-shared-pointer-contents.rst | 2 +- .../bugprone/move-shared-pointer-contents.cpp | 7 +++++++ 4 files changed, 19 insertions(+), 19 deletions(-) diff --git a/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.cpp index b4a393b7f2f200..9730cf2f0603c7 100644 --- a/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.cpp @@ -23,13 +23,6 @@ MoveSharedPointerContentsCheck::MoveSharedPointerContentsCheck( SharedPointerClasses(utils::options::parseStringList( Options.get("SharedPointerClasses", "std::shared_ptr"))) {} -MoveSharedPointerContentsCheck::~MoveSharedPointerContentsCheck() = default; - -bool MoveSharedPointerContentsCheck::isLanguageVersionSupported( - const LangOptions &LangOptions) const { - return LangOptions.CPlusPlus11; -} - void MoveSharedPointerContentsCheck::registerMatchers(MatchFinder *Finder) { Finder->addMatcher( callExpr( @@ -48,13 +41,11 @@ void MoveSharedPointerContentsCheck::check( const auto *Call = Result.Nodes.getNodeAs<CallExpr>("call"); const auto *Op = Result.Nodes.getNodeAs<Expr>("op"); - if (Op) { - diag(Call->getBeginLoc(), - "don't move the contents out of a shared pointer, as other accessors " - "expect them to remain in a determinate state") - << FixItHint::CreateInsertion(Call->getBeginLoc(), "*") - << FixItHint::CreateRemoval(Op->getBeginLoc()); - } + diag(Call->getBeginLoc(), + "don't move the contents out of a shared pointer, as other accessors " + "expect them to remain in a determinate state") + << FixItHint::CreateInsertion(Call->getBeginLoc(), "*") + << FixItHint::CreateRemoval(Op->getBeginLoc()); } } // namespace clang::tidy::bugprone diff --git a/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.h b/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.h index a9ae8d335941e8..71b826596f23cc 100644 --- a/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.h +++ b/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.h @@ -10,23 +10,25 @@ #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_MOVESHAREDPOINTERCONTENTSCHECK_H #include <vector> - #include "../ClangTidyCheck.h" +#include "llvm/ADT/StringRef.h" namespace clang::tidy::bugprone { -/// FIXME: Write a short description. +/// 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)``. /// /// 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); - ~MoveSharedPointerContentsCheck() override; void registerMatchers(ast_matchers::MatchFinder *Finder) override; void check(const ast_matchers::MatchFinder::MatchResult &Result) override; bool - isLanguageVersionSupported(const LangOptions &LangOptions) const override; + isLanguageVersionSupported(const LangOptions &LangOptions) const override { + return LangOptions.CPlusPlus11; + } private: const std::vector<StringRef> SharedPointerClasses; 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 4536663a1b7470..bf7edcf74fbb61 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 @@ -14,4 +14,4 @@ Options .. option :: SharedPointerClasses A semicolon-separated list of class names that should be treated as shared - pointers. By default only `std::shared_ptr` is included. + pointers. Default is `std::shared_ptr`. 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 0837708a7171c5..5f1980213c6be0 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 @@ -66,3 +66,10 @@ void multiStars() { // CHECK-FIXES: *std::move(p) } // CHECK-MESSAGES: :[[@LINE-3]]: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] + +// This case should be caught, but isn't. +template <typename T> +void unresolved() { + std::shared_ptr<T> p; + T x = 2 * std::move(*p) * 3; +} >From 415e5518119537ee820485fdf7353e7265644758 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/5] [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-tidy/bugprone/MoveSharedPointerContentsCheck.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.h b/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.h index 71b826596f23cc..681dfe7f4202b9 100644 --- a/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.h +++ b/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.h @@ -9,9 +9,9 @@ #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" #include "llvm/ADT/StringRef.h" +#include <vector> namespace clang::tidy::bugprone { >From 2d0fd03b76a7b86925526dc802d955e70993624b 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/5] [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-tidy/bugprone/CMakeLists.txt | 1 - .../MoveSharedPointerContentsCheck.cpp | 24 ++++---- .../bugprone/MoveSharedPointerContentsCheck.h | 3 + .../docs/clang-tidy/checks/list.rst | 3 +- .../bugprone/move-shared-pointer-contents.cpp | 55 ++++++++++++------- 5 files changed, 49 insertions(+), 37 deletions(-) diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt index e92b03ddfa883e..10be385a8de09c 100644 --- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt @@ -25,7 +25,6 @@ add_clang_library(clangTidyBugproneModule ImplicitWideningOfMultiplicationResultCheck.cpp InaccurateEraseCheck.cpp IncorrectEnableIfCheck.cpp - MoveSharedPointerContentsCheck.cpp SwitchMissingDefaultCaseCheck.cpp IncDecInConditionsCheck.cpp IncorrectRoundingsCheck.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.cpp index 9730cf2f0603c7..92882885961f17 100644 --- a/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.cpp @@ -21,17 +21,19 @@ 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"))) {} void MoveSharedPointerContentsCheck::registerMatchers(MatchFinder *Finder) { Finder->addMatcher( - callExpr( - callee(functionDecl(hasName("std::move"))), - hasArgument(0, cxxOperatorCallExpr(hasOverloadedOperatorName("*"), - callee(cxxMethodDecl(ofClass( - matchers::matchesAnyListedName( - SharedPointerClasses))))) - .bind("op"))) + callExpr(anyOf(callee(functionDecl(hasName("::std::move"))), + callee(unresolvedLookupExpr(hasAnyDeclaration(namedDecl( + hasUnderlyingDecl(hasName("::std::move"))))))), + hasArgument(0, anyOf(cxxOperatorCallExpr( + hasOverloadedOperatorName("*"), + callee(cxxMethodDecl(ofClass( + matchers::matchesAnyListedName( + SharedPointerClasses))))), + unaryOperator(hasOperatorName("*"))))) .bind("call"), this); } @@ -39,13 +41,9 @@ void MoveSharedPointerContentsCheck::registerMatchers(MatchFinder *Finder) { void MoveSharedPointerContentsCheck::check( const MatchFinder::MatchResult &Result) { const auto *Call = Result.Nodes.getNodeAs<CallExpr>("call"); - const auto *Op = Result.Nodes.getNodeAs<Expr>("op"); - diag(Call->getBeginLoc(), "don't move the contents out of a shared pointer, as other accessors " - "expect them to remain in a determinate state") - << FixItHint::CreateInsertion(Call->getBeginLoc(), "*") - << FixItHint::CreateRemoval(Op->getBeginLoc()); + "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 index 681dfe7f4202b9..88fc860b25c459 100644 --- a/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.h +++ b/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.h @@ -29,6 +29,9 @@ class MoveSharedPointerContentsCheck : public ClangTidyCheck { isLanguageVersionSupported(const LangOptions &LangOptions) const override { return LangOptions.CPlusPlus11; } + std::optional<TraversalKind> getCheckTraversalKind() const override { + return TK_AsIs; + } private: const std::vector<StringRef> SharedPointerClasses; diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index aaafa89cdbd594..5ea59b226fb329 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -107,7 +107,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-move-shared-pointer-contents <bugprone/move-shared-pointer-contents>`, :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>`, @@ -119,7 +119,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-shared-pointer-contents-move <bugprone/shared-pointer-contents-move>`, "Yes" :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>`, 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 5f1980213c6be0..1ad7bfe7105160 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 @@ -33,43 +33,56 @@ template <typename T> using OtherSharedPtr = std::shared_ptr<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<int> p; - int x = *std::move(p); + std::shared_ptr<Nontrivial> p; + Nontrivial x = *std::move(p); } void simpleFinding() { - std::shared_ptr<int> p; - int y = std::move(*p); - // CHECK-FIXES: *std::move(p) + std::shared_ptr<Nontrivial> p; + Nontrivial y = std::move(*p); } -// CHECK-MESSAGES: :[[@LINE-3]]:11: 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] +// 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 int_ptr = std::shared_ptr<int>; - int_ptr p; - int x = std::move(*p); - // CHECK-FIXES: *std::move(p) + using nontrivial_ptr = std::shared_ptr<Nontrivial>; + nontrivial_ptr p; + Nontrivial x = std::move(*p); } -// CHECK-MESSAGES: :[[@LINE-3]]:11: 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] +// 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<int> p; - int x = std::move(*p); - // CHECK-FIXES: *std::move(p) + my::OtherSharedPtr<Nontrivial> p; + Nontrivial x = std::move(*p); } -// CHECK-MESSAGES: :[[@LINE-3]]:11: 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] +// 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 multiStars() { - std::shared_ptr<int> p; - int x = 2 * std::move(*p) * 3; - // CHECK-FIXES: *std::move(p) + std::shared_ptr<Nontrivial> p; + int x = 2 * std::move(*p).x * 3; } -// CHECK-MESSAGES: :[[@LINE-3]]: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] +// 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] -// This case should be caught, but isn't. template <typename T> void unresolved() { std::shared_ptr<T> p; - T x = 2 * std::move(*p) * 3; + 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] + +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] >From ca778d6ee12ad12e2095934ddf46a9c606b33b2c Mon Sep 17 00:00:00 2001 From: David Pizzuto <piz...@google.com> Date: Tue, 26 Sep 2023 10:45:42 -0700 Subject: [PATCH 5/5] [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-tidy/bugprone/CMakeLists.txt | 1 - .../MoveSharedPointerContentsCheck.cpp | 130 ++++++++++++++++-- .../bugprone/MoveSharedPointerContentsCheck.h | 7 + .../bugprone/move-shared-pointer-contents.rst | 12 +- .../docs/clang-tidy/checks/list.rst | 3 +- .../bugprone/move-shared-pointer-contents.cpp | 93 ++++++++++--- 6 files changed, 204 insertions(+), 42 deletions(-) diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt index e92b03ddfa883e..10be385a8de09c 100644 --- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt @@ -25,7 +25,6 @@ add_clang_library(clangTidyBugproneModule ImplicitWideningOfMultiplicationResultCheck.cpp InaccurateEraseCheck.cpp IncorrectEnableIfCheck.cpp - MoveSharedPointerContentsCheck.cpp SwitchMissingDefaultCaseCheck.cpp IncDecInConditionsCheck.cpp IncorrectRoundingsCheck.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.cpp index 9730cf2f0603c7..ae3944ba5d26df 100644 --- a/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.cpp @@ -21,31 +21,137 @@ 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"))) {} void MoveSharedPointerContentsCheck::registerMatchers(MatchFinder *Finder) { + auto isStdMove = callee(functionDecl(hasName("::std::move"))); + + // Resolved type, direct move. Finder->addMatcher( - callExpr( - callee(functionDecl(hasName("std::move"))), - hasArgument(0, cxxOperatorCallExpr(hasOverloadedOperatorName("*"), + callExpr(isStdMove, hasArgument(0, cxxOperatorCallExpr( + hasOverloadedOperatorName("*"), callee(cxxMethodDecl(ofClass( matchers::matchesAnyListedName( - SharedPointerClasses))))) - .bind("op"))) + 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; + 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 auto *Call = Result.Nodes.getNodeAs<CallExpr>("call"); - const auto *Op = Result.Nodes.getNodeAs<Expr>("op"); + const auto *GetCall = Result.Nodes.getNodeAs<CallExpr>("get_call"); + const auto *UnresolvedCall = + Result.Nodes.getNodeAs<CallExpr>("unresolved_call"); + const auto *UnresolvedPtr = Result.Nodes.getNodeAs<QualType>("unresolved_p"); + const auto *UnresolvedGetCall = + Result.Nodes.getNodeAs<CallExpr>("unresolved_get_call"); + const auto *UnresolvedGetPtr = + Result.Nodes.getNodeAs<VarDecl>("unresolved_get_p"); + + const bool Unresolved = isSharedPointerClass(UnresolvedPtr); + const bool UnresolvedGet = isSharedPointerClass(UnresolvedGetPtr); - diag(Call->getBeginLoc(), - "don't move the contents out of a shared pointer, as other accessors " - "expect them to remain in a determinate state") - << FixItHint::CreateInsertion(Call->getBeginLoc(), "*") - << FixItHint::CreateRemoval(Op->getBeginLoc()); + clang::SourceLocation Loc; + if (Unresolved) { + Loc = UnresolvedCall->getBeginLoc(); + } else if (UnresolvedGet) { + Loc = UnresolvedGetCall->getBeginLoc(); + } else if (GetCall != nullptr) { + Loc = GetCall->getBeginLoc(); + } else if (Call != nullptr) { + Loc = Call->getBeginLoc(); + } + 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 index 681dfe7f4202b9..46cf31f53d315f 100644 --- a/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.h +++ b/clang-tools-extra/clang-tidy/bugprone/MoveSharedPointerContentsCheck.h @@ -29,8 +29,15 @@ class MoveSharedPointerContentsCheck : public ClangTidyCheck { 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 QualType *QT) const; + bool isSharedPointerClass(const VarDecl *VD) const; + const std::vector<StringRef> SharedPointerClasses; }; 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 bf7edcf74fbb61..53cd861557e21c 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 @@ -3,10 +3,11 @@ 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)``. Other -reference holders may not be expecting the move and suddenly getting empty or -otherwise indeterminate states can cause issues. +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 ------- @@ -14,4 +15,5 @@ Options .. option :: SharedPointerClasses A semicolon-separated list of class names that should be treated as shared - pointers. Default is `std::shared_ptr`. + 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 aaafa89cdbd594..5ea59b226fb329 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -107,7 +107,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-move-shared-pointer-contents <bugprone/move-shared-pointer-contents>`, :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>`, @@ -119,7 +119,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-shared-pointer-contents-move <bugprone/shared-pointer-contents-move>`, "Yes" :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>`, 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 5f1980213c6be0..d4cc75cdc2f939 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,4 +1,4 @@ -// 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;'}}" // Some dummy definitions we'll need. @@ -31,45 +31,94 @@ struct shared_ptr { 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<int> p; - int x = *std::move(p); + std::shared_ptr<Nontrivial> p; + Nontrivial x = *std::move(p); } void simpleFinding() { - std::shared_ptr<int> p; - int y = std::move(*p); - // CHECK-FIXES: *std::move(p) + std::shared_ptr<Nontrivial> p; + Nontrivial y = std::move(*p); } -// CHECK-MESSAGES: :[[@LINE-3]]:11: 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] +// 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 int_ptr = std::shared_ptr<int>; - int_ptr p; - int x = std::move(*p); - // CHECK-FIXES: *std::move(p) + using nontrivial_ptr = std::shared_ptr<Nontrivial>; + nontrivial_ptr p; + Nontrivial x = std::move(*p); } -// CHECK-MESSAGES: :[[@LINE-3]]:11: 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] +// 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<int> p; - int x = std::move(*p); - // CHECK-FIXES: *std::move(p) + 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); } -// CHECK-MESSAGES: :[[@LINE-3]]:11: 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 multiStars() { - std::shared_ptr<int> p; - int x = 2 * std::move(*p) * 3; - // CHECK-FIXES: *std::move(p) + std::shared_ptr<Nontrivial> p; + int x = 2 * std::move(*p).x * 3; } -// CHECK-MESSAGES: :[[@LINE-3]]: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] +// 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] -// This case should be caught, but isn't. template <typename T> void unresolved() { std::shared_ptr<T> p; - T x = 2 * std::move(*p) * 3; + 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); } _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits