Skip to content

[clang-tidy][NFC] Enable 'performance-move-const-arg' in '.clang-tidy' config #148549

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

vbvictor
Copy link
Contributor

Set performance-move-const-arg.CheckTriviallyCopyableMove option to false because "trivially copyable" is too strict and give warning for e.g. MixData class:

Here:
Mix(const ParmVarDecl *F, const ParmVarDecl *S, MixData Data)
: First(F), Second(S), Data(std::move(Data)) {}

I find std::move here useful, WDYT?

With option set to true (default) we get these more warnings which are only about trivially copyable types:

diff --git a/clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
index a179d4bf66b4..5217dd4a50b6 100644
--- a/clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
@@ -502,7 +502,7 @@ struct Mix {
   MixData Data;
 
   Mix(const ParmVarDecl *F, const ParmVarDecl *S, MixData Data)
-      : First(F), Second(S), Data(std::move(Data)) {}
+      : First(F), Second(S), Data(Data) {}
 
   void sanitize() { Data.sanitize(); }
   MixFlags flags() const { return Data.Flags; }
@@ -1476,7 +1476,7 @@ static MixableParameterRange modelMixingRange(
       assert(M.flagsValid() && "All flags decayed!");
 
       if (M.mixable())
-        MixesOfIth.emplace_back(std::move(M));
+        MixesOfIth.emplace_back(M);
     }
 
     if (MixesOfIth.empty()) {
diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp
index 0b6b8d9c9713..5c2d97b20518 100644
--- a/clang-tools-extra/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp
+++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp
@@ -132,7 +132,7 @@ void SpecialMemberFunctionsCheck::check(
     llvm::SmallVectorImpl<SpecialMemberFunctionData> &Members =
         ClassWithSpecialMembers[ID];
     if (!llvm::is_contained(Members, Data))
-      Members.push_back(std::move(Data));
+      Members.push_back(Data);
   };
 
   if (const auto *Dtor = Result.Nodes.getNodeAs<CXXMethodDecl>("dtor")) {
diff --git a/clang-tools-extra/clang-tidy/modernize/UseConstraintsCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseConstraintsCheck.cpp
index e9b96c4016af..a506e01a637b 100644
--- a/clang-tools-extra/clang-tidy/modernize/UseConstraintsCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/UseConstraintsCheck.cpp
@@ -145,7 +145,7 @@ matchEnableIfSpecialization(TypeLoc TheType) {
     TheType = Qualified.getUnqualifiedLoc();
 
   if (auto EnableIf = matchEnableIfSpecializationImpl(TheType))
-    return EnableIfData{std::move(*EnableIf), TheType};
+    return EnableIfData{*EnableIf, TheType};
   return std::nullopt;
 }
 
diff --git a/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp
index 2af67f7ccb4c..7b1c2b4cb839 100644
--- a/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp
@@ -38,7 +38,7 @@ namespace {
 struct NotLengthExprForStringNode {
   NotLengthExprForStringNode(std::string ID, DynTypedNode Node,
                              ASTContext *Context)
-      : ID(std::move(ID)), Node(std::move(Node)), Context(Context) {}
+      : ID(std::move(ID)), Node(Node), Context(Context) {}
   bool operator()(const internal::BoundNodesMap &Nodes) const {
     // Match a string literal and an integer size or strlen() call.
     if (const auto *StringLiteralNode = Nodes.getNodeAs<StringLiteral>(ID)) {
diff --git a/clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp b/clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
index fab2365f1147..6f94d991a4e1 100644
--- a/clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
@@ -280,7 +280,7 @@ IdentifierNamingCheck::FileStyle IdentifierNamingCheck::getFileStyleFromOptions(
         Options.get<IdentifierNamingCheck::CaseType>(StyleString);
 
     if (CaseOptional || Prefix || Postfix || IgnoredRegexpStr || HPTOpt)
-      Styles[I].emplace(std::move(CaseOptional), Prefix.value_or(""),
+      Styles[I].emplace(CaseOptional, Prefix.value_or(""),
                         Postfix.value_or(""), IgnoredRegexpStr.value_or(""),
                         HPTOpt.value_or(IdentifierNamingCheck::HPT_Off));
   }

@llvmbot
Copy link
Member

llvmbot commented Jul 13, 2025

@llvm/pr-subscribers-clang-tidy

@llvm/pr-subscribers-clang-tools-extra

Author: Baranov Victor (vbvictor)

Changes

Set performance-move-const-arg.CheckTriviallyCopyableMove option to false because "trivially copyable" is too strict and give warning for e.g. MixData class:

Here:
Mix(const ParmVarDecl *F, const ParmVarDecl *S, MixData Data)
: First(F), Second(S), Data(std::move(Data)) {}

I find std::move here useful, WDYT?

With option set to true (default) we get these more warnings which are only about trivially copyable types:

diff --git a/clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
index a179d4bf66b4..5217dd4a50b6 100644
--- a/clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
@@ -502,7 +502,7 @@ struct Mix {
   MixData Data;
 
   Mix(const ParmVarDecl *F, const ParmVarDecl *S, MixData Data)
-      : First(F), Second(S), Data(std::move(Data)) {}
+      : First(F), Second(S), Data(Data) {}
 
   void sanitize() { Data.sanitize(); }
   MixFlags flags() const { return Data.Flags; }
@@ -1476,7 +1476,7 @@ static MixableParameterRange modelMixingRange(
       assert(M.flagsValid() &amp;&amp; "All flags decayed!");
 
       if (M.mixable())
-        MixesOfIth.emplace_back(std::move(M));
+        MixesOfIth.emplace_back(M);
     }
 
     if (MixesOfIth.empty()) {
diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp
index 0b6b8d9c9713..5c2d97b20518 100644
--- a/clang-tools-extra/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp
+++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp
@@ -132,7 +132,7 @@ void SpecialMemberFunctionsCheck::check(
     llvm::SmallVectorImpl&lt;SpecialMemberFunctionData&gt; &amp;Members =
         ClassWithSpecialMembers[ID];
     if (!llvm::is_contained(Members, Data))
-      Members.push_back(std::move(Data));
+      Members.push_back(Data);
   };
 
   if (const auto *Dtor = Result.Nodes.getNodeAs&lt;CXXMethodDecl&gt;("dtor")) {
diff --git a/clang-tools-extra/clang-tidy/modernize/UseConstraintsCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseConstraintsCheck.cpp
index e9b96c4016af..a506e01a637b 100644
--- a/clang-tools-extra/clang-tidy/modernize/UseConstraintsCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/UseConstraintsCheck.cpp
@@ -145,7 +145,7 @@ matchEnableIfSpecialization(TypeLoc TheType) {
     TheType = Qualified.getUnqualifiedLoc();
 
   if (auto EnableIf = matchEnableIfSpecializationImpl(TheType))
-    return EnableIfData{std::move(*EnableIf), TheType};
+    return EnableIfData{*EnableIf, TheType};
   return std::nullopt;
 }
 
diff --git a/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp
index 2af67f7ccb4c..7b1c2b4cb839 100644
--- a/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp
@@ -38,7 +38,7 @@ namespace {
 struct NotLengthExprForStringNode {
   NotLengthExprForStringNode(std::string ID, DynTypedNode Node,
                              ASTContext *Context)
-      : ID(std::move(ID)), Node(std::move(Node)), Context(Context) {}
+      : ID(std::move(ID)), Node(Node), Context(Context) {}
   bool operator()(const internal::BoundNodesMap &amp;Nodes) const {
     // Match a string literal and an integer size or strlen() call.
     if (const auto *StringLiteralNode = Nodes.getNodeAs&lt;StringLiteral&gt;(ID)) {
diff --git a/clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp b/clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
index fab2365f1147..6f94d991a4e1 100644
--- a/clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
@@ -280,7 +280,7 @@ IdentifierNamingCheck::FileStyle IdentifierNamingCheck::getFileStyleFromOptions(
         Options.get&lt;IdentifierNamingCheck::CaseType&gt;(StyleString);
 
     if (CaseOptional || Prefix || Postfix || IgnoredRegexpStr || HPTOpt)
-      Styles[I].emplace(std::move(CaseOptional), Prefix.value_or(""),
+      Styles[I].emplace(CaseOptional, Prefix.value_or(""),
                         Postfix.value_or(""), IgnoredRegexpStr.value_or(""),
                         HPTOpt.value_or(IdentifierNamingCheck::HPT_Off));
   }

Full diff: https://github.com/llvm/llvm-project/pull/148549.diff

5 Files Affected:

  • (modified) clang-tools-extra/clang-tidy/.clang-tidy (+4-1)
  • (modified) clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.cpp (+1-2)
  • (modified) clang-tools-extra/clang-tidy/modernize/AvoidCArraysCheck.cpp (+1-1)
  • (modified) clang-tools-extra/clang-tidy/modernize/UseStdNumbersCheck.cpp (+1-1)
  • (modified) clang-tools-extra/clang-tidy/utils/UseRangesCheck.cpp (+1-1)
diff --git a/clang-tools-extra/clang-tidy/.clang-tidy b/clang-tools-extra/clang-tidy/.clang-tidy
index 2443c979621da..be8e598de9634 100644
--- a/clang-tools-extra/clang-tidy/.clang-tidy
+++ b/clang-tools-extra/clang-tidy/.clang-tidy
@@ -16,7 +16,6 @@ Checks: >
   -modernize-use-trailing-return-type,
   performance-*,
   -performance-enum-size,
-  -performance-move-const-arg,
   -performance-no-int-to-ptr,
   -performance-type-promotion-in-math-fn,
   -performance-unnecessary-value-param,
@@ -39,3 +38,7 @@ Checks: >
   -readability-static-definition-in-anonymous-namespace,
   -readability-suspicious-call-argument,
   -readability-use-anyofallof
+
+CheckOptions:
+  - key:             performance-move-const-arg.CheckTriviallyCopyableMove
+    value:           false
diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.cpp
index 82fd3316b942a..82d1cf13440bc 100644
--- a/clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.cpp
+++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.cpp
@@ -132,8 +132,7 @@ void MissingStdForwardCheck::registerMatchers(MatchFinder *Finder) {
           hasAncestor(functionDecl().bind("func")),
           hasAncestor(functionDecl(
               isDefinition(), equalsBoundNode("func"), ToParam,
-              unless(anyOf(isDeleted(),
-                           hasDescendant(std::move(ForwardCallMatcher))))))),
+              unless(anyOf(isDeleted(), hasDescendant(ForwardCallMatcher)))))),
       this);
 }
 
diff --git a/clang-tools-extra/clang-tidy/modernize/AvoidCArraysCheck.cpp b/clang-tools-extra/clang-tidy/modernize/AvoidCArraysCheck.cpp
index 0804aa76d953c..ee70911dff1d5 100644
--- a/clang-tools-extra/clang-tidy/modernize/AvoidCArraysCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/AvoidCArraysCheck.cpp
@@ -67,7 +67,7 @@ void AvoidCArraysCheck::registerMatchers(MatchFinder *Finder) {
                            hasParent(fieldDecl(
                                hasParent(recordDecl(isExternCContext())))),
                            hasAncestor(functionDecl(isExternC())))),
-              std::move(IgnoreStringArrayIfNeededMatcher))
+              IgnoreStringArrayIfNeededMatcher)
           .bind("typeloc"),
       this);
 }
diff --git a/clang-tools-extra/clang-tidy/modernize/UseStdNumbersCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseStdNumbersCheck.cpp
index ff5a302dfa75d..934cc24817d73 100644
--- a/clang-tools-extra/clang-tidy/modernize/UseStdNumbersCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/UseStdNumbersCheck.cpp
@@ -319,7 +319,7 @@ void UseStdNumbersCheck::registerMatchers(MatchFinder *const Finder) {
 
   Finder->addMatcher(
       expr(
-          anyOfExhaustive(std::move(ConstantMatchers)),
+          anyOfExhaustive(ConstantMatchers),
           unless(hasParent(explicitCastExpr(hasDestinationType(isFloating())))),
           hasType(qualType(hasCanonicalTypeUnqualified(
               anyOf(qualType(asString("float")).bind("float"),
diff --git a/clang-tools-extra/clang-tidy/utils/UseRangesCheck.cpp b/clang-tools-extra/clang-tidy/utils/UseRangesCheck.cpp
index e421c9f11b24b..25601f9a01a48 100644
--- a/clang-tools-extra/clang-tidy/utils/UseRangesCheck.cpp
+++ b/clang-tools-extra/clang-tidy/utils/UseRangesCheck.cpp
@@ -149,7 +149,7 @@ void UseRangesCheck::registerMatchers(MatchFinder *Finder) {
     }
     Finder->addMatcher(
         callExpr(
-            callee(functionDecl(hasAnyName(std::move(Names)))
+            callee(functionDecl(hasAnyName(Names))
                        .bind((FuncDecl + Twine(Replacers.size() - 1).str()))),
             ast_matchers::internal::DynTypedMatcher::constructVariadic(
                 ast_matchers::internal::DynTypedMatcher::VO_AnyOf,

Copy link
Contributor

@carlosgalvezp carlosgalvezp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants