[clang-tools-extra] [clang-tidy] Add performance-move-smart-pointer-contents check. (PR #66139)
via cfe-commits
cfe-commits at lists.llvm.org
Tue Sep 26 11:13:21 PDT 2023
https://github.com/pizzud updated https://github.com/llvm/llvm-project/pull/66139
>From b699129b21c95571410a809d16fdf8cfcf1526c5 Mon Sep 17 00:00:00 2001
From: David Pizzuto <pizzud at google.com>
Date: Tue, 12 Sep 2023 13:24:48 -0700
Subject: [PATCH 1/3] [clang-tidy] Add performance-move-smart-pointer-contents
check.
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.
---
.../clang-tidy/performance/CMakeLists.txt | 1 +
.../MoveSmartPointerContentsCheck.cpp | 80 ++++++++++++
.../MoveSmartPointerContentsCheck.h | 39 ++++++
.../performance/PerformanceTidyModule.cpp | 3 +
clang-tools-extra/docs/ReleaseNotes.rst | 5 +
.../docs/clang-tidy/checks/list.rst | 1 +
.../move-smart-pointer-contents.rst | 23 ++++
.../move-smart-pointer-contents.cpp | 119 ++++++++++++++++++
8 files changed, 271 insertions(+)
create mode 100644 clang-tools-extra/clang-tidy/performance/MoveSmartPointerContentsCheck.cpp
create mode 100644 clang-tools-extra/clang-tidy/performance/MoveSmartPointerContentsCheck.h
create mode 100644 clang-tools-extra/docs/clang-tidy/checks/performance/move-smart-pointer-contents.rst
create mode 100644 clang-tools-extra/test/clang-tidy/checkers/performance/move-smart-pointer-contents.cpp
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)
>From ca5c9bb013d0012b2900af78c21eb7391436774d Mon Sep 17 00:00:00 2001
From: David Pizzuto <pizzud at google.com>
Date: Tue, 12 Sep 2023 13:24:48 -0700
Subject: [PATCH 2/3] [clang-tidy] Add performance-move-smart-pointer-contents
check.
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.
---
.../MoveSmartPointerContentsCheck.cpp | 73 +++++++++++--------
.../MoveSmartPointerContentsCheck.h | 3 +-
2 files changed, 45 insertions(+), 31 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/performance/MoveSmartPointerContentsCheck.cpp b/clang-tools-extra/clang-tidy/performance/MoveSmartPointerContentsCheck.cpp
index 8c1170641e5c718..2a1f16b39362983 100644
--- a/clang-tools-extra/clang-tidy/performance/MoveSmartPointerContentsCheck.cpp
+++ b/clang-tools-extra/clang-tidy/performance/MoveSmartPointerContentsCheck.cpp
@@ -8,9 +8,15 @@
#include <string>
+<<<<<<< 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.)
#include "clang/AST/ASTContext.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
#include "clang/Lex/Lexer.h"
@@ -19,15 +25,23 @@ using namespace clang::ast_matchers;
namespace clang::tidy::performance {
+bool MoveSmartPointerContentsCheck::isLanguageVersionSupported(
+ const LangOptions &LangOptions) const {
+ if (!LangOptions.CPlusPlus) {
+ return false;
+ }
+
+ // Anything after C++11 is fine.
+ return LangOptions.CPlusPlus11;
+}
+
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))) {}
+ Options.get("SharedPointerClasses", "std::shared_ptr"))) {}
void MoveSmartPointerContentsCheck::storeOptions(
ClangTidyOptions::OptionMap &Opts) {
@@ -41,39 +55,40 @@ 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"),
+ hasArgument(
+ 0,
+ anyOf(cxxOperatorCallExpr(hasOperatorName("*"),
+ hasDeclaration(cxxMethodDecl(namedDecl(
+ matchers::matchesAnyListedName(
+ UniquePointerClasses)))))
+ .bind("unique_op"),
+ cxxOperatorCallExpr(hasOperatorName("*"),
+ hasDeclaration(cxxMethodDecl(namedDecl(
+ matchers::matchesAnyListedName(
+ SharedPointerClasses)))))
+ .bind("shared_op"))))
+ .bind("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");
+ const auto *Call = Result.Nodes.getNodeAs<CallExpr>("call");
+ const auto *UniqueOp = Result.Nodes.getNodeAs<Expr>("unique_op");
+ const auto *SharedOp = Result.Nodes.getNodeAs<Expr>("shared_op");
- diag(UniqueCall->getBeginLoc(),
- "prefer to move the smart pointer rather than its contents") << FixItHint::CreateInsertion(UniqueCall->getBeginLoc(), "*")
- << FixItHint::CreateRemoval(UniqueOp->getBeginLoc());
+ if (UniqueOp) {
+ diag(Call->getBeginLoc(),
+ "prefer to move the smart pointer rather than its contents")
+ << FixItHint::CreateInsertion(Call->getBeginLoc(), "*")
+ << FixItHint::CreateRemoval(UniqueOp->getBeginLoc());
}
- if (SharedCall) {
- const auto* SharedOp = Result.Nodes.getNodeAs<Expr>("shared_op");
-
- diag(SharedCall->getBeginLoc(),
+ if (SharedOp) {
+ 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(SharedCall->getBeginLoc(), "*")
- << FixItHint::CreateRemoval(SharedOp->getBeginLoc());
+ "expect them to remain in a determinate state")
+ << FixItHint::CreateInsertion(Call->getBeginLoc(), "*")
+ << FixItHint::CreateRemoval(SharedOp->getBeginLoc());
}
}
diff --git a/clang-tools-extra/clang-tidy/performance/MoveSmartPointerContentsCheck.h b/clang-tools-extra/clang-tidy/performance/MoveSmartPointerContentsCheck.h
index 65bede0e164afc7..8b9e8e9d2b3065d 100644
--- a/clang-tools-extra/clang-tidy/performance/MoveSmartPointerContentsCheck.h
+++ b/clang-tools-extra/clang-tidy/performance/MoveSmartPointerContentsCheck.h
@@ -26,12 +26,11 @@ class MoveSmartPointerContentsCheck : public ClangTidyCheck {
void registerMatchers(ast_matchers::MatchFinder *Finder) override;
void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+ bool isLanguageVersionSupported(const LangOptions &LangOpts) const 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
>From ae3aa65618a5a8ed52948bd86bae0cf1ecdd020f Mon Sep 17 00:00:00 2001
From: David Pizzuto <pizzud at google.com>
Date: Tue, 12 Sep 2023 13:24:48 -0700
Subject: [PATCH 3/3] [clang-tidy] Add performance-move-smart-pointer-contents
check.
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.
---
.../MoveSmartPointerContentsCheck.cpp | 48 +++--------------
.../MoveSmartPointerContentsCheck.h | 3 +-
.../move-smart-pointer-contents.rst | 4 +-
.../move-smart-pointer-contents.cpp | 53 +------------------
4 files changed, 11 insertions(+), 97 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/performance/MoveSmartPointerContentsCheck.cpp b/clang-tools-extra/clang-tidy/performance/MoveSmartPointerContentsCheck.cpp
index 2a1f16b39362983..efe8de2d70f5a94 100644
--- a/clang-tools-extra/clang-tidy/performance/MoveSmartPointerContentsCheck.cpp
+++ b/clang-tools-extra/clang-tidy/performance/MoveSmartPointerContentsCheck.cpp
@@ -8,15 +8,9 @@
#include <string>
-<<<<<<< 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.)
#include "clang/AST/ASTContext.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
#include "clang/Lex/Lexer.h"
@@ -27,11 +21,6 @@ namespace clang::tidy::performance {
bool MoveSmartPointerContentsCheck::isLanguageVersionSupported(
const LangOptions &LangOptions) const {
- if (!LangOptions.CPlusPlus) {
- return false;
- }
-
- // Anything after C++11 is fine.
return LangOptions.CPlusPlus11;
}
@@ -39,34 +28,22 @@ MoveSmartPointerContentsCheck::MoveSmartPointerContentsCheck(
StringRef Name, ClangTidyContext *Context)
: ClangTidyCheck(Name, Context),
UniquePointerClasses(utils::options::parseStringList(
- Options.get("UniquePointerClasses", "std::unique_ptr"))),
- SharedPointerClasses(utils::options::parseStringList(
- Options.get("SharedPointerClasses", "std::shared_ptr"))) {}
+ Options.get("UniquePointerClasses", "std::unique_ptr"))) {}
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,
- anyOf(cxxOperatorCallExpr(hasOperatorName("*"),
- hasDeclaration(cxxMethodDecl(namedDecl(
- matchers::matchesAnyListedName(
- UniquePointerClasses)))))
- .bind("unique_op"),
- cxxOperatorCallExpr(hasOperatorName("*"),
- hasDeclaration(cxxMethodDecl(namedDecl(
- matchers::matchesAnyListedName(
- SharedPointerClasses)))))
- .bind("shared_op"))))
+ hasArgument(0, cxxOperatorCallExpr(hasOverloadedOperatorName("*"),
+ callee(cxxMethodDecl(ofClass(
+ matchers::matchesAnyListedName(
+ UniquePointerClasses)))))))
.bind("call"),
this);
}
@@ -74,21 +51,10 @@ void MoveSmartPointerContentsCheck::registerMatchers(MatchFinder *Finder) {
void MoveSmartPointerContentsCheck::check(
const MatchFinder::MatchResult &Result) {
const auto *Call = Result.Nodes.getNodeAs<CallExpr>("call");
- const auto *UniqueOp = Result.Nodes.getNodeAs<Expr>("unique_op");
- const auto *SharedOp = Result.Nodes.getNodeAs<Expr>("shared_op");
- if (UniqueOp) {
- diag(Call->getBeginLoc(),
- "prefer to move the smart pointer rather than its contents")
- << FixItHint::CreateInsertion(Call->getBeginLoc(), "*")
- << FixItHint::CreateRemoval(UniqueOp->getBeginLoc());
- }
- if (SharedOp) {
+ if (Call) {
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(SharedOp->getBeginLoc());
+ "prefer to move the smart pointer rather than its contents");
}
}
diff --git a/clang-tools-extra/clang-tidy/performance/MoveSmartPointerContentsCheck.h b/clang-tools-extra/clang-tidy/performance/MoveSmartPointerContentsCheck.h
index 8b9e8e9d2b3065d..0466e54c8847c59 100644
--- a/clang-tools-extra/clang-tidy/performance/MoveSmartPointerContentsCheck.h
+++ b/clang-tools-extra/clang-tidy/performance/MoveSmartPointerContentsCheck.h
@@ -15,7 +15,7 @@
namespace clang::tidy::performance {
-/// Checks that std::move is not called on the contents of a smart pointer and
+/// Checks that std::move is not called on the contents of a unique pointer and
/// suggests moving out of the pointer instead.
///
/// For the user-facing documentation see:
@@ -30,7 +30,6 @@ class MoveSmartPointerContentsCheck : public ClangTidyCheck {
private:
const std::vector<StringRef> UniquePointerClasses;
- const std::vector<StringRef> SharedPointerClasses;
};
} // namespace clang::tidy::performance
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
index 03d72d7250ff7a2..65e936b344490e1 100644
--- 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
@@ -6,8 +6,8 @@ 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.
+`*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.
Options
-------
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
index ebc4dc683fb16d4..47def271fd3c84d 100644
--- 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
@@ -1,9 +1,7 @@
// 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;'}}"
+// RUN: 'std::unique_ptr; my::OtherUniquePtr;'}}"
// Some dummy definitions we'll need.
@@ -31,24 +29,12 @@ struct unique_ptr {
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() {
@@ -61,7 +47,6 @@ void simpleFindingUnique() {
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>;
@@ -69,51 +54,15 @@ void aliasedUniqueType() {
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)
More information about the cfe-commits
mailing list