[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 12 16:16:09 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 cfe-commits
mailing list