[clang-tidy] Add performance-move-smart-pointer-contents check. #66139
Conversation
This check detects moves of the contents of a smart pointer as opposed to the smart pointer itself. For unique_ptr this is a performance issue, as the underlying type is presumably not cheap to move, but for shared_ptr it's actually dangerous as other code that also has access to the shared_ptr is probably not expecting the move. The set of "unique" and "shared" pointer classes are configurable via options to allow individual projects to add additional classes of each type.
|
@llvm/pr-subscribers-clang-tidy ChangesThis check detects moves of the contents of a smart pointer as opposed to the smart pointer itself. For unique_ptr this is a performance issue, as the underlying type is presumably not cheap to move, but for shared_ptr it's actually dangerous as other code that also has access to the shared_ptr is probably not expecting the move.The set of "unique" and "shared" pointer classes are configurable via options to allow individual projects to add additional classes of each type.Full diff: https://github.com/llvm/llvm-project/pull/66139.diff 8 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/performance/CMakeLists.txt b/clang-tools-extra/clang-tidy/performance/CMakeLists.txt
index 81128ff086021ed..35435a951c0717b 100644
--- a/clang-tools-extra/clang-tidy/performance/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/performance/CMakeLists.txt
@@ -14,6 +14,7 @@ add_clang_library(clangTidyPerformanceModule
InefficientVectorOperationCheck.cpp
MoveConstArgCheck.cpp
MoveConstructorInitCheck.cpp
+ MoveSmartPointerContentsCheck.cpp
NoAutomaticMoveCheck.cpp
NoIntToPtrCheck.cpp
NoexceptDestructorCheck.cpp
diff --git a/clang-tools-extra/clang-tidy/performance/MoveSmartPointerContentsCheck.cpp b/clang-tools-extra/clang-tidy/performance/MoveSmartPointerContentsCheck.cpp
new file mode 100644
index 000000000000000..8c1170641e5c718
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/performance/MoveSmartPointerContentsCheck.cpp
@@ -0,0 +1,80 @@
+//===--- MoveSmartPointerContentsCheck.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 <string>
+
+#include "MoveSmartPointerContentsCheck.h"
+#include "../utils/Matchers.h"
+#include "../utils/OptionsUtils.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::performance {
+
+MoveSmartPointerContentsCheck::MoveSmartPointerContentsCheck(
+ StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context),
+ UniquePointerClasses(utils::options::parseStringList(
+ Options.get("UniquePointerClasses", "std::unique_ptr"))),
+ IsAUniquePointer(namedDecl(hasAnyName(UniquePointerClasses))),
+ SharedPointerClasses(utils::options::parseStringList(
+ Options.get("SharedPointerClasses", "std::shared_ptr"))),
+ IsASharedPointer(namedDecl(hasAnyName(SharedPointerClasses))) {}
+
+void MoveSmartPointerContentsCheck::storeOptions(
+ ClangTidyOptions::OptionMap &Opts) {
+ Options.store(Opts, "UniquePtrClasses",
+ utils::options::serializeStringList(UniquePointerClasses));
+ Options.store(Opts, "SharedPtrClasses",
+ utils::options::serializeStringList(SharedPointerClasses));
+}
+
+void MoveSmartPointerContentsCheck::registerMatchers(MatchFinder *Finder) {
+ Finder->addMatcher(
+ callExpr(
+ callee(functionDecl(hasName("std::move"))),
+ hasArgument(0, cxxOperatorCallExpr(hasOperatorName("*"),
+ hasDeclaration(cxxMethodDecl(ofClass(IsAUniquePointer)))).bind("unique_op")))
+ .bind("unique_call"),
+ this);
+
+ Finder->addMatcher(
+ callExpr(
+ callee(functionDecl(hasName("std::move"))),
+ hasArgument(0, cxxOperatorCallExpr(hasOperatorName("*"),
+ hasDeclaration(cxxMethodDecl(ofClass(IsASharedPointer)))).bind("shared_op")))
+ .bind("shared_call"),
+ this);
+}
+
+void MoveSmartPointerContentsCheck::check(
+ const MatchFinder::MatchResult &Result) {
+ const auto *UniqueCall = Result.Nodes.getNodeAs<CallExpr>("unique_call");
+ const auto *SharedCall = Result.Nodes.getNodeAs<CallExpr>("shared_call");
+
+ if (UniqueCall) {
+ const auto* UniqueOp = Result.Nodes.getNodeAs<Expr>("unique_op");
+
+ diag(UniqueCall->getBeginLoc(),
+ "prefer to move the smart pointer rather than its contents") << FixItHint::CreateInsertion(UniqueCall->getBeginLoc(), "*")
+ << FixItHint::CreateRemoval(UniqueOp->getBeginLoc());
+ }
+ if (SharedCall) {
+ const auto* SharedOp = Result.Nodes.getNodeAs<Expr>("shared_op");
+
+ diag(SharedCall->getBeginLoc(),
+ "don't move the contents out of a shared pointer, as other accessors "
+ "expect them to remain in a determinate state") << FixItHint::CreateInsertion(SharedCall->getBeginLoc(), "*")
+ << FixItHint::CreateRemoval(SharedOp->getBeginLoc());
+ }
+}
+
+} // namespace clang::tidy::performance
diff --git a/clang-tools-extra/clang-tidy/performance/MoveSmartPointerContentsCheck.h b/clang-tools-extra/clang-tidy/performance/MoveSmartPointerContentsCheck.h
new file mode 100644
index 000000000000000..65bede0e164afc7
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/performance/MoveSmartPointerContentsCheck.h
@@ -0,0 +1,39 @@
+//===--- MoveSmartPointerContentsCheck.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_PERFORMANCE_MOVESMARTPOINTERCONTENTSCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_MOVESMARTPOINTERCONTENTSCHECK_H
+
+#include <vector>
+
+#include "../ClangTidyCheck.h"
+
+namespace clang::tidy::performance {
+
+/// Checks that std::move is not called on the contents of a smart pointer and
+/// suggests moving out of the pointer instead.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/performance/move-smart-pointer-contents.html
+class MoveSmartPointerContentsCheck : public ClangTidyCheck {
+public:
+ MoveSmartPointerContentsCheck(StringRef Name, ClangTidyContext *Context);
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+ void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+
+private:
+ const std::vector<StringRef> UniquePointerClasses;
+ const ast_matchers::internal::Matcher<RecordDecl> IsAUniquePointer;
+ const std::vector<StringRef> SharedPointerClasses;
+ const ast_matchers::internal::Matcher<RecordDecl> IsASharedPointer;
+};
+
+} // namespace clang::tidy::performance
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_MOVESMARTPOINTERCONTENTSCHECK_H
diff --git a/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp b/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
index 9e0fa6f88b36a00..53842dfe4c2bd59 100644
--- a/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
@@ -19,6 +19,7 @@
#include "InefficientVectorOperationCheck.h"
#include "MoveConstArgCheck.h"
#include "MoveConstructorInitCheck.h"
+#include "MoveSmartPointerContentsCheck.h"
#include "NoAutomaticMoveCheck.h"
#include "NoIntToPtrCheck.h"
#include "NoexceptDestructorCheck.h"
@@ -53,6 +54,8 @@ class PerformanceModule : public ClangTidyModule {
"performance-move-const-arg");
CheckFactories.registerCheck<MoveConstructorInitCheck>(
"performance-move-constructor-init");
+ CheckFactories.registerCheck<MoveSmartPointerContentsCheck>(
+ "performance-move-smart-pointer-contents");
CheckFactories.registerCheck<NoAutomaticMoveCheck>(
"performance-no-automatic-move");
CheckFactories.registerCheck<NoIntToPtrCheck>("performance-no-int-to-ptr");
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 19c977977f9044c..27c1b996c6ca049 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -168,6 +168,11 @@ New checks
Recommends the smallest possible underlying type for an ``enum`` or ``enum``
class based on the range of its enumerators.
+- New :doc:`performance-move-smart-pointer-contents
+ <clang-tidy/checks/performance/move-smart-pointer-contents>` check.
+
+ Recommends moving a smart pointer rather than its contents.
+
- New :doc:`readability-reference-to-constructed-temporary
<clang-tidy/checks/readability/reference-to-constructed-temporary>` check.
diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst
index 75a86431d264412..7c6770e17250203 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -317,6 +317,7 @@ Clang-Tidy Checks
:doc:`performance-inefficient-vector-operation <performance/inefficient-vector-operation>`, "Yes"
:doc:`performance-move-const-arg <performance/move-const-arg>`, "Yes"
:doc:`performance-move-constructor-init <performance/move-constructor-init>`,
+ :doc:`performance-move-smart-pointer-contents <performance/move-smart-pointer-contents>`, "Yes"
:doc:`performance-no-automatic-move <performance/no-automatic-move>`,
:doc:`performance-no-int-to-ptr <performance/no-int-to-ptr>`,
:doc:`performance-noexcept-destructor <performance/noexcept-destructor>`, "Yes"
diff --git a/clang-tools-extra/docs/clang-tidy/checks/performance/move-smart-pointer-contents.rst b/clang-tools-extra/docs/clang-tidy/checks/performance/move-smart-pointer-contents.rst
new file mode 100644
index 000000000000000..03d72d7250ff7a2
--- /dev/null
+++ b/clang-tools-extra/docs/clang-tidy/checks/performance/move-smart-pointer-contents.rst
@@ -0,0 +1,23 @@
+.. title:: clang-tidy - performance-move-smart-pointer-contents
+
+performance-move-smart-pointer-contents
+=======================================
+
+Given a smart pointer containing a movable type, such as a
+`std::unique_ptr<SomeProtocolBuffer>`, it's possible to move the contents of the
+pointer rather than the pointer itself (ie `std::move(*p)` rather than
+`*std::move(p)`). Doing so is a pessimization, as if the type could be efficiently
+moved we wouldn't need to put it in a `unique_ptr` to begin with.
+
+Options
+-------
+
+.. option :: UniquePointerClasses
+
+ A semicolon-separated list of class names that should be treated as unique
+ pointers. By default only `std::unique_ptr` is included.
+
+.. 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/test/clang-tidy/checkers/performance/move-smart-pointer-contents.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance/move-smart-pointer-contents.cpp
new file mode 100644
index 000000000000000..ebc4dc683fb16d4
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/performance/move-smart-pointer-contents.cpp
@@ -0,0 +1,119 @@
+// RUN: %check_clang_tidy %s performance-move-smart-pointer-contents %t -- \
+// RUN: -config="{CheckOptions: \
+// RUN: {performance-move-smart-pointer-contents.UniquePointerClasses: \
+// RUN: 'std::unique_ptr; my::OtherUniquePtr;',\
+// RUN: performance-move-smart-pointer-contents.SharedPointerClasses: \
+// RUN: '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 unique_ptr {
+ unique_ptr();
+ T *get() const;
+ explicit operator bool() const;
+ void reset(T *ptr);
+ T &operator*() const;
+ T *operator->() const;
+ T& operator[](size_t i) const;
+};
+
+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 OtherUniquePtr = std::unique_ptr<T>;
+template <typename T>
+using OtherSharedPtr = std::shared_ptr<T>;
+} // namespace my
+
+void correctUnique() {
+ std::unique_ptr<int> p;
+ int x = *std::move(p);
+}
+
+void simpleFindingUnique() {
+ std::unique_ptr<int> p;
+ int x = std::move(*p);
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:11: warning: prefer to move the smart pointer rather than its contents [performance-move-smart-pointer-contents]
+// CHECK-FIXES: *std::move(p)
+
+void aliasedUniqueType() {
+ using int_ptr = std::unique_ptr<int>;
+ int_ptr p;
+ int x = std::move(*p);
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:11: warning: prefer to move the smart pointer rather than its contents [performance-move-smart-pointer-contents]
+// CHECK-FIXES: *std::move(p)
+
+void configWorksUnique() {
+ my::OtherUniquePtr<int> p;
+ int x = std::move(*p);
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:11: warning: prefer to move the smart pointer rather than its contents [performance-move-smart-pointer-contents]
+// CHECK-FIXES: *std::move(p)
+
+void multiStarsUnique() {
+ std::unique_ptr<int> p;
+ int x = 2 * std::move(*p) * 3;
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:15: warning: prefer to move the smart pointer rather than its contents [performance-move-smart-pointer-contents]
+// CHECK-FIXES: *std::move(p)
+
+void correctShared() {
+ std::shared_ptr<int> p;
+ int x = *std::move(p);
+}
+
+void simpleFindingShared() {
+ std::shared_ptr<int> p;
+ int y = std::move(*p);
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:11: warning: don't move the contents out of a shared pointer, as other accessors expect them to remain in a determinate state [performance-move-smart-pointer-contents]
+
+void aliasedSharedType() {
+ using int_ptr = std::shared_ptr<int>;
+ int_ptr p;
+ int x = std::move(*p);
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:11: warning: don't move the contents out of a shared pointer, as other accessors expect them to remain in a determinate state [performance-move-smart-pointer-contents]
+// CHECK-FIXES: *std::move(p)
+
+void configWorksShared() {
+ my::OtherSharedPtr<int> p;
+ int x = std::move(*p);
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:11: warning: don't move the contents out of a shared pointer, as other accessors expect them to remain in a determinate state [performance-move-smart-pointer-contents]
+// CHECK-FIXES: *std::move(p)
+
+void multiStarsShared() {
+ std::shared_ptr<int> p;
+ int x = 2 * std::move(*p) * 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 [performance-move-smart-pointer-contents]
+// CHECK-FIXES: *std::move(p)
|
clang-tools-extra/clang-tidy/performance/MoveSmartPointerContentsCheck.cpp
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/performance/MoveSmartPointerContentsCheck.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/performance/MoveSmartPointerContentsCheck.h
Show resolved
Hide resolved
There was a problem hiding this comment.
I don't see problem described by this check as an performance issue.
For example:
std::unique_ptr<std::vector<int>> ptr;
std::vector<int> local = std::move(*ptr);
No performance issue here, simply value may need to be moved, do not expect that someone have to always keep object in unique_ptr, at some point it may need to be moved/copied.
And some types may be cheap to move.
As for std::shared_ptr, yes this could be a problem.
My suggestion is:
- Part related to moving object from a std::shared_ptr should be a bugprone-moved-from-shared check (or some other name).
- As for current check, I fear that it may cause lot of false-positives, and basicly it should work only on "heavy" types, but it could be hard to detect such heavy types. But still I don't think why that part should be reduced to just shared ptr, check that would detect std::move calls that are still heavy, simply because moved class is above XYZ bytes in size.
Fix provided by this check *std::move(p) is also questionable... Move will still be called, and this check will still detect such issue, in that case fix does not fix anything.
| `std::unique_ptr<SomeProtocolBuffer>`, it's possible to move the contents of the | ||
| pointer rather than the pointer itself (ie `std::move(*p)` rather than | ||
| `*std::move(p)`). Doing so is a pessimization, as if the type could be efficiently | ||
| moved we wouldn't need to put it in a `unique_ptr` to begin with. |
There was a problem hiding this comment.
"Doing so is a pessimization, as if the type could be efficiently
moved we wouldn't need to put it in a unique_ptr to begin with."
You do not know a reasons why someone used unique_ptr, maybe they use custom deleter, maybe they follow some guidelines, maybe they want object to be on heap, not on stack.
@martinboehme internally suggested putting this in the performance category; perhaps he has a more detailed rationale or some relevant thoughts?
This check won't re-flag it (see the slipped through bugprone-use-after-move because it assumes the |
| pointer rather than the pointer itself (ie `std::move(*p)` rather than | ||
| `*std::move(p)`). Doing so is a pessimization if the type cannot be efficiently | ||
| moved, as the pointer will be quicker than a larger type. | ||
|
|
There was a problem hiding this comment.
I think the performance issue will happened only when SomeProtocolBuffer is is_trivially_move_constructible and is a large type.
You will not get any benefit from
std::unique_ptr<int> p;
int x = std::move(*p);to
std::unique_ptr<int> p;
int x = *std::move(p);
There was a problem hiding this comment.
This check has issue in my opinion, the semantic of std::move(*p) is not same as *std::move(p)
#include <iostream>
#include <memory>
#include <string>
using std::unique_ptr;
void f1() {
unique_ptr<std::string> p{new std::string("demo")};
std::cout << p.get() << " *p=" << *p << "\n";
auto x = std::move(*p);
std::cout << p.get() << " *p=" << *p << " x=" << x << "\n";
}
void f2() {
unique_ptr<std::string> p{new std::string("demo")};
std::cout << p.get() << " *p=" << *p << "\n";
auto x = *std::move(p);
std::cout << p.get() << " *p=" << *p << " x=" << x << "\n";
}
int main() {
std::cout << "std::move(*p)\n";
f1();
std::cout << "*std::move(p)\n";
f2();
}The output is
std::move(*p)
0x603000001b40 *p=demo
0x603000001b40 *p= x=demo
*std::move(p)
0x603000001b70 *p=demo
0x603000001b70 *p=demo x=demo
In *std::move(p), move constructor does not happen, it is same as *p.
While it's true, as @PiotrZSL points out here and in another comment, that a class being "heavyweight" (i.e. expensive to move) is not the only reason to put it behind a Quoting a counterexample from a different comment: std::unique_ptr<std::vector<int>> ptr;
std::vector<int> local = std::move(*ptr);Why would someone ever put a So I think that if we want to recommend to the user to do
Footnotes
|
Agree that the suggested fix may not always be correct. I would suggest simply not emitting a suggested fix (i.e. only emitting a warning). |
| <<<<<<< HEAD | ||
| #include "MoveSmartPointerContentsCheck.h" | ||
| #include "../utils/Matchers.h" | ||
| #include "../utils/OptionsUtils.h" | ||
| ======= | ||
| #include "../utils/Matchers.h" | ||
| #include "../utils/OptionsUtils.h" | ||
| #include "MoveSmartPointerContentsCheck.h" | ||
| >>>>>>> b0179a7f3cac ([clang-tidy] Add performance-move-smart-pointer-contents check.) |
There was a problem hiding this comment.
It looks as if you've got remnants of a merge conflict here?
| if (!LangOptions.CPlusPlus) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
If you're checking for CPlusPlus11 anyway, I believe this is redundant.
| anyOf(cxxOperatorCallExpr(hasOperatorName("*"), | ||
| hasDeclaration(cxxMethodDecl(namedDecl( | ||
| matchers::matchesAnyListedName( | ||
| UniquePointerClasses))))) |
There was a problem hiding this comment.
I believe the canonical way of doing this is:
cxxOperatorCallExpr(hasOverloadedOperatorName("*"),
callee(cxxMethodDecl(ofClass(matchers::matchesAnyListedName(
UniquePointerClasses)))));
| // CHECK-MESSAGES: :[[@LINE-2]]:11: warning: prefer to move the smart pointer rather than its contents [performance-move-smart-pointer-contents] | ||
| // CHECK-FIXES: *std::move(p) |
There was a problem hiding this comment.
Prefer to put this directly below the line that triggers the diagnostic
|
Other common example from me: And next one: And next one: Simply moving a content of unique ptr is not an issue for me, coping big structure is. Simply when for shared_ptr such construction raises red flags, for unique_ptr is normal, and may have nothing to do with performance. For performance better would be check that actually check if move operation is cheap or expensive. Consider extracting shared_ptr part into separate check, finish that one and then we could think about unique_ptr approach. |
clang-tools-extra/docs/clang-tidy/checks/performance/move-smart-pointer-contents.rst
Show resolved
Hide resolved
clang-tools-extra/docs/clang-tidy/checks/performance/move-smart-pointer-contents.rst
Outdated
Show resolved
Hide resolved
clang-tools-extra/docs/clang-tidy/checks/performance/move-smart-pointer-contents.rst
Outdated
Show resolved
Hide resolved
|
Extracted the shared_ptr parts to #67467. I'll continue the unique_ptr portion here and respond to those comments later today. |
This check detects moves of the contents of a smart pointer as opposed to the smart pointer itself. For unique_ptr this is a performance issue, as the underlying type is presumably not cheap to move, but for shared_ptr it's actually dangerous as other code that also has access to the shared_ptr is probably not expecting the move. The set of "unique" and "shared" pointer classes are configurable via options to allow individual projects to add additional classes of each type.
|
After looking at the various comments (thanks all!) it feels like the consensus was:
Is there a reasonable way to detect expensive moves? Looking for trivially-moveable seems like a decent start, but it's hard to know when a given instance of a type may be large (eg long strings). |
|
Personally I think that check for move out of std::unique_ptr, and check for overall heavy moves could be implemented as an separate one, not related to unique_ptr. Easiest way to detect heavy moves is to check size of object, and above some threshold mark them as heavy. But thats a separate problem. Maybe this PR should be abandoned. |
|
I've been kind of leaving this in limbo while we sort out the shared_ptr one and I agree it probably makes more sense to make bugprone-unique-pointer-move as a parallel to shared-pointer in PR #67467 and consider performance-heavy-move separately, since there's runtime-vs-compile-time considerations there (eg sizeof(std::string) vs s.size()). Let's drop this and I'll open up new PRs with each additional check. |
| performance-move-smart-pointer-contents | ||
| ======================================= | ||
|
|
||
| Recommends avoiding moving out of a smart pointer when moving the pointer is |
There was a problem hiding this comment.
Please make this statement same as in Release Notes.
| cheaper. | ||
|
|
||
| Given a smart pointer containing a movable type, such as a | ||
| ``std::unique_ptr<SomeProtocolBuffer>``, it's possible to move the contents of the |
There was a problem hiding this comment.
Please follow 80 characters limit. Same below.
This check detects moves of the contents of a smart pointer as opposed to the smart pointer itself. For unique_ptr this is a performance issue, as the underlying type is presumably not cheap to move, but for shared_ptr it's actually dangerous as other code that also has access to the shared_ptr is probably not expecting the move.
The set of "unique" and "shared" pointer classes are configurable via options to allow individual projects to add additional classes of each type.