[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