[Openmp-commits] [openmp] [clang-tidy] Add performance-move-smart-pointer-contents check. (PR #66139)

via Openmp-commits openmp-commits at lists.llvm.org
Tue Sep 12 16:16:07 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] [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)



More information about the Openmp-commits mailing list