[clang-tidy] Fix wrong FixIt in performance-move-const-arg

[Sockke]

Summary

There are incorrect Fixit and missing warnings:
case :
A trivially-copyable object wrapped by std::move is passed to the function with rvalue reference parameters. Removing std::move will cause compilation errors.

void showInt(int&&) {}
void testInt() {
    int a = 10;
    // expect: warning + nofix
    showInt(std::move(a));  // showInt(a) <--- wrong fix
}

struct Tmp {};
void showTmp(Tmp&&) {}
void testTmp() {
    Tmp t;
    // expect: warning + nofix
    showTmp(std::move(t));  // showTmp(t) <--- wrong fix
}

[Sockke]

format.

[• Quuxplusone]

clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp

75

You're right that some diagnostic should be given here, but this is not the right diagnostic. The issue is not that the std::move is non-functional, but rather that it's superfluous; a move will happen, with or without the std::move. (And the std::move prevents move elision, so removing it may actually improve codegen.)

257

This warning is incorrect, as indicated in your PR summary. Isn't the point of this PR mainly to remove this incorrect warning?

[MTC]

clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp

75

Correct! @Sockke would you self-examination these cases again? I have checked this case, see https://godbolt.org/z/3ch3sbG17.

257

I guess what @Sockke wants to express is that applying std::move on the integer type makes no sense.

[whisperity]

clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp

51

I am not sure if ignoringImplicit(ignoringParenCasts(_)) is the same as ignoringParenImpCasts(_), but I've seen the latter one appear many times in other matchers.

clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp

257

Yes, but there is no FixIt for this warning, but even if the user fixes this manually according to the warning, it'll result in

error: cannot bind rvalue reference of type int&& to lvalue of type int

because showInt takes int&&!

So this is not just unfixable automatically (hence no CHECK-FIXES line), it isn't fixable at all.

[• Quuxplusone]

clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp

257

At a really high level, yeah, this code is bad code. :) But this is exactly Sockke's "case 1":

A trivially-copyable object wrapped by std::move is passed to the function with rvalue reference parameters. Removing std::move will cause compilation errors.

So a warning here would have to be, like, "std::move of a, of trivially copyable type int, has no runtime effect; consider changing showInt's parameter from int&& to int."
Also, should add a test case of the form

void showInt(int&&);
template<class T> void forwardToShowInt(T&& t) { showInt(static_cast<T&&>(t)); }
void test() {
    int a = 10;
    forwardToShowInt(std::move(a));
}

to make sure the diagnostic (or lack thereof) plays well with templates. Here, refactoring forwardToShowInt's parameter from T&& to T is unlikely to be a realistic option.

[whisperity]

clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp

257

Yes, I was thinking the same, maybe it could warn about the function (in a note or something). But then we reach the point that the check's name is fuzzy at best... (It has an already outdated name, though.)

[Sockke]

Thank you for your reviews, @Quuxplusone, @whisperity, @MTC. I was also considering adding new diagnoses to the current check for these special cases, but I am not sure how to classify and describe them correctly, and whether they need to be added to the new check.
First of all, I want to make sure that the timing of warning and FxiIt in these cases is correct? If the above is no problem, I think I need to classify and modify the diagnostic information in the next step, is that right?

[Sockke]

update!

[MTC]

clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp

257

Change 'int'&& -> 'int&&' and 'int&' -> int.

Make consider changing showInt's parameter from 'int'&& to 'int'& as a note instead of a warning. And I don't have a strong preference for the position of the note, but I personally want to put it in the source location of the function definition. and

[• Quuxplusone]

I'm not at all convinced that any of this complexity is worth it. Have you run this check on any large codebase to see how many false positives it has? Does it catch any true positives?

clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp

280

Here, the more immediate red flag is that the programmer is using std::move on an rvalue expression.

std::string&& a();
std::string& b();
std::string c();
auto aa = std::move(a());  // not great, moving an xvalue
auto bb = std::move(b());  // OK, moving an lvalue
auto cc = std::move(c());  // not great, moving a prvalue

However, the more complicated and guess-worky these diagnostics get, the more I want to see how they interact with templates and generic code.

template<class F>
auto w(F f) {
    return std::move(f());
}
std::string callw1() { std::string s; return w([&]() -> std::string { return s; });
std::string callw2() { std::string s; return w([&]() -> std::string& { return s; });

callw1 ends up applying std::move to a prvalue inside w, but when callw2 calls w, the std::move actually does something. Possible strategies here include "Warn on code that's only sometimes correct," "Don't warn on code that's only sometimes wrong," and "Don't implement the diagnostic at all because it's too icky."

283

This suggestion seems bad. If Tmp&& was originally a typo, then the machine can't know what the programmer might have meant. If Tmp&& was trying to indicate "take ownership of," then the right fix would probably be to pass Tmp by value.

[MTC]

I think we're a bit off track, and @Sockke wants to accomplish more than one goal in the same patch. I have summarized what we are currently discussing as follow shows:

1. Fix the wrong AutoFix which blocks the compilation.
2. Find more 'unrecommended' std::move usage and give correct warning messages.
3. Whether template should be taken into account.

In addition, I would like to mention that we need to ensure that this check should be consistent with -Wpessimizing-move, see https://reviews.llvm.org/D7633, which has done the perfect job.

I suggest that this patch be divided into two patches. In the current patch, fix the wrong AutoFix. What the current check should look like is left in the second patch for discussion. @Sockke do you mind simplifying this patch and only achieving the first goal?

clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp

280

Agree! For Tmp t2 = std::move(Tmp());, what should be reported here is moving a prvalue. However, the original behavior is emitting this message.

And the original matcher will ignore template instantiation, see unless(isInTemplateInstantiation()).

[Sockke]

In D107450#2938824, @MTC wrote:

I think we're a bit off track, and @Sockke wants to accomplish more than one goal in the same patch. I have summarized what we are currently discussing as follow shows:

1. Fix the wrong AutoFix which blocks the compilation.
2. Find more 'unrecommended' std::move usage and give correct warning messages.
3. Whether template should be taken into account.

In addition, I would like to mention that we need to ensure that this check should be consistent with -Wpessimizing-move, see https://reviews.llvm.org/D7633, which has done the perfect job.

I suggest that this patch be divided into two patches. In the current patch, fix the wrong AutoFix. What the current check should look like is left in the second patch for discussion. @Sockke do you mind simplifying this patch and only achieving the first goal?

Yes, I originally wanted to make some improvements on the premise of keeping the old version of the idea. As Quuxplusone said, this improvement still does not meet the final requirements, which may require rewriting the entire checker. Any other thoughts? @Quuxplusone,@aaron.ballman,@whisperity,@alexfh

[• Quuxplusone]

I suggest that this patch be divided into two patches. In the current patch, fix the wrong AutoFix. What the current check should look like is left in the second patch for discussion. @Sockke do you mind simplifying this patch and only achieving the first goal?

FWIW, I agree. The original "case 1" was fixing wrong behavior; the original "case 2" was adding a new feature. Let's see just the "case 1" bugfix (and regression test), and then once that's being properly handled and regression-tested, it'll be safer to mess around with new feature work.

[Sockke]

Fix the wrong AutoFix which blocks the compilation.

[• Quuxplusone]

clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp

263

forwardToShowInt takes T&&, not int&&, and so it can't be changed in the way the diagnostic is suggesting. I think the right answer here is not to emit the diagnostic at all, when the offending function is a template.

(Relatively minor nits, all moot because this diagnostic should not be emitted at all: 'int'&& should be 'int&&', trivially copyable should not be hyphenated, and int& is a strictly worse suggestion than either const int& or plain int. IMVHO it would be reasonable to land a very trivial patch to remove the hyphen from trivially copyable and update the tests, and then rebase this PR on top of that.)

271

This is precisely the bogus message you're trying to eliminate, isn't it? Removing std::move would make the code fail to compile because t is an lvalue but showTmp requires an rvalue.

[Sockke]

update! Fixed some diagnostic descriptions.

[• Quuxplusone]

clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp

263

Okay, I have no complaints about the diagnostics being tested by these tests anymore.
I am puzzled about why the tool is suggesting to rewrite int&& as int, but not suggesting to rewrite Tmp&& as Tmp; those cases should be exactly isomorphic, right?
But the goal of this PR is now to eliminate wrong fixits, and I believe you've succeeded at that.
Btw, I'm not qualified to review the actual code change in MoveConstArgCheck.cpp; I'll let someone else do that.

[Sockke]

Thanks for your reply @Quuxplusone. I have updated!

[Sockke]

This comment was removed by Sockke.

clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp

263

Keep the original diagnostic behavior, but without "remove std::move" message.

263

Thank you for your reply! Yes, they should be exactly isomorphic when Tmp is a trivially copyable type.

[Sockke]

Any thoughts? : )

[• Quuxplusone]

clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp

262–263

forwardToShowInt(std::move(a));
// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect

I continue to want-not-to-block this PR, since I think it's improving the situation. However, FWIW...
It's good that this message doesn't contain a fixit, since there's nothing the programmer can really do to "fix" this call. But then, should this message be emitted at all? If this were clang -Wall, then we definitely would not want to emit a "noisy" warning where there's basically nothing the programmer can do about it. Does clang-tidy have a similar philosophy? Or is it okay for clang-tidy to say "This code looks wonky" even when there's no obvious way to fix it?

[Sockke]

Which do you prefer? @Quuxplusone

clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp

262–263

forwardToShowInt(std::move(a));
// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect

I continue to want-not-to-block this PR, since I think it's improving the situation. However, FWIW...
It's good that this message doesn't contain a fixit, since there's nothing the programmer can really do to "fix" this call. But then, should this message be emitted at all? If this were clang -Wall, then we definitely would not want to emit a "noisy" warning where there's basically nothing the programmer can do about it. Does clang-tidy have a similar philosophy? Or is it okay for clang-tidy to say "This code looks wonky" even when there's no obvious way to fix it?

Yes, I agree with you. I did not remove warning to maintain the original behavior, which will make the current patch clearer. In any case, it is easy for me to make this modification if you want.

[• Quuxplusone]

clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp

262–263

I'll defer on this subject to whomever you get to review the code change in MoveConstArgCheck.cpp. (@whisperity perhaps?)

[whisperity]

I'm a bit confused about this, but it has been a while since I read this patch. Am I right to think that the code in the patch and the original submission (the patch summary) diverged since the review started? I do not see anything "removed" from the test files, only a new addition. Or did you accidentally upload a wrong diff somewhere? The original intention of the patch was to remove bogus printouts.

In general: the test file ends without any testing for functions with multiple parameters. Are they out-of-scope by design?

clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp

143–144

+? Shouldn't this be ||? I know they're equivalent over Z_2, but if we're using && instead of * then let's adhere to the style.

143–144

clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp

257

In D107450#2936824, @MTC wrote:

but I personally want to put it in the source location of the function definition

(I think you submitted the comment too early, @MTC, as there's an unterminated sentence there.)

But I agree with this. It should be put there when we're talking about the function's signature. The reason being the developer reading the warning could dismiss a potential false positive earlier if they can also immediately read (at least a part of...) the function's signature.

@Sockke you can do this by doing another diag() call with the Note value given as the 3rd parameter. And you can specify the SourceLocation of the affected parameter for this note.

262–263

(Negative, it would be @aaron.ballman or @alexfh or the other maintainers. I'm just trying to give back to the community after taking away a lot of reviewer effort with a humongous check that I just recently pushed in after years of (re-)development.)

262–263

In D107450#2958900, @Quuxplusone wrote:

It's good that this message doesn't contain a fixit, since there's nothing the programmer can really do to "fix" this call. But then, should this message be emitted at all?

As an individual not representing but myself, I would say no. At least not in a check which is not sold as a style-like or guideline-enforcement analysis implementation.

If this were clang -Wall, then we definitely would not want to emit a "noisy" warning where there's basically nothing the programmer can do about it. Does clang-tidy have a similar philosophy? Or is it okay for clang-tidy to say "This code looks wonky" even when there's no obvious way to fix it?

I think in general tools should have this policy. 😇

267–270

Is there a test case covering if there are separate invocations of the function? Just to make sure that the consider changing the parameter suggestion won't become an annoyance either, and result in conflicts with other parts of the code.

271

We could ensure that the symbol name is printed the same way as the other placeholder-injected named decls.

[Sockke]

Thanks for your review, I greatly appreciate it. @whisperity
There is no wrong case in the original test file, which is why we did not catch the bug in the test suite. From this, I added some new cases.
I tested the functions with multi-parameters, there is no problem, I will add them to the test file.

In D107450#2968708, @whisperity wrote:

I'm a bit confused about this, but it has been a while since I read this patch. Am I right to think that the code in the patch and the original submission (the patch summary) diverged since the review started? I do not see anything "removed" from the test files, only a new addition. Or did you accidentally upload a wrong diff somewhere? The original intention of the patch was to remove bogus printouts.

In general: the test file ends without any testing for functions with multiple parameters. Are they out-of-scope by design?

clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp

143–144

143–144

+? Shouldn't this be ||? I know they're equivalent over Z_2, but if we're using && instead of * then let's adhere to the style.

The reason for this is that the maximum capacity of diag is 10.

clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp

257

In D107450#2936824, @MTC wrote:

but I personally want to put it in the source location of the function definition

(I think you submitted the comment too early, @MTC, as there's an unterminated sentence there.)

But I agree with this. It should be put there when we're talking about the function's signature. The reason being the developer reading the warning could dismiss a potential false positive earlier if they can also immediately read (at least a part of...) the function's signature.

@Sockke you can do this by doing another diag() call with the Note value given as the 3rd parameter. And you can specify the SourceLocation of the affected parameter for this note.

Yes, it is reasonable to add a note, but would you mind adding this modification to the patch that fixes AutoFix? If you don't mind, I will update it.

267–270

The warning will only be given when calling this function and passing in the parameters wrapped with std::move. Is my understanding correct？

271

Yes, this is a better implementation.

[Sockke]

kindly ping.

[Sockke]

Hi, Could you please take some time to review this diff again? @whisperity

[whisperity]

clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp

254

From int &? Isn't the parameter int && there?

257

I think it is okay if the additional note for this is added in this patch, there is no need for a separate patch on that. It helps understand WHY the FixIt is wrong or not wrong, anyway.

267–270

Yes, that is what I mean. But I would like to see a test case where the "target function" is called multiple times with different arguments. One call with std::move(x), one with a direct temporary, etc. So we can make sure that if a function is called more than 1 time, we can still be sure there won't be bad warnings or bad fixits that contradict each other.

[Sockke]

Sorry for the delayed reply because of National Day. I have updated. @whisperity

[Sockke]

Kindly ping.

[Sockke]

Hi, Could anyone please review this diff? @whisperity, @aaron.ballman

[Sockke]

Kindly ping. @whisperity, @aaron.ballman, @Quuxplusone

[whisperity]

clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp

177

clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp

260

[whisperity]

I think this is becoming okay and looks safe enough. I can't really grasp what HasCheckedMoveSet means, and how it synergises with storing CallExprs so maybe a better name should be added. Do you mean AlreadyCheckedMoves? When is it possible that the same CallExpr of std::move will be matched multiple times?

In general going from T&& to const T& would be a broken change if the function implementation calls a non-const member method on the T instance, but I can be convinced that saying consider [...] instead of generating a bogus fix-it is enough, there is hopefully no need to extensively analyse and guard against this.

There has been a new release since the patch was proposed so a rebase should be needed.
@aaron.ballman What do you think, does this warrant a ReleaseNotes entry?

[aaron.ballman]

In D107450#3087103, @whisperity wrote:

There has been a new release since the patch was proposed so a rebase should be needed.
@aaron.ballman What do you think, does this warrant a ReleaseNotes entry?

I think a release note for this would be helpful, yes!

[Sockke]

Update!

[Sockke]

In D107450#3087103, @whisperity wrote:

I think this is becoming okay and looks safe enough. I can't really grasp what HasCheckedMoveSet means, and how it synergises with storing CallExprs so maybe a better name should be added. Do you mean AlreadyCheckedMoves? When is it possible that the same CallExpr of std::move will be matched multiple times?

Yes, you're right!

[whisperity]

Okay. I think I am convinced. And removing a bogus automated fix is always a positive change!

clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp

181

I am not sure if this will always return something reasonable as a printed name.

Could you please add a test case where the receiving function is a constructor? E.g. a testStringView that takes, let's say, const char*&&?

clang-tools-extra/docs/ReleaseNotes.rst

159–162

I tried to make the release note entry a bit easier to understand. Of course, please reflow!

[Sockke]

I improved the logic of judgments needing to report suggesting notes in some cases. And more tests were added here.

[Sockke]

In addition, the splicing logics of "FunctionName" and "ExpectParmTypeName" were improved.

[Sockke]

Kindly ping. @whisperity

[Sockke]

Kindly ping. @whisperity

[Sockke]

Hi, Could anyone please review this diff? @whisperity, @aaron.ballman

[whisperity]

This generally looks fine for me, but I'd rather have other people who are more knowledgeable about the standard's intricacies looked at it too.

AFAIK Aaron is busy this week and the next (?) due to C++ Committee meetings, or something like that. And I bet after such meetings everyone would take a few more days off from looking at C++ stuff.

[Sockke]

In D107450#3155019, @whisperity wrote:

This generally looks fine for me, but I'd rather have other people who are more knowledgeable about the standard's intricacies looked at it too.

AFAIK Aaron is busy this week and the next (?) due to C++ Committee meetings, or something like that. And I bet after such meetings everyone would take a few more days off from looking at C++ stuff.

Ok, that sounds great, thanks a lot!

[Sockke]

Kindly ping. @aaron.ballman

[aaron.ballman]

clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp

182

Not all CallExpr objects have a direct callee, so this will crash (such as calls through a function pointer rather than a direct function call). It may be worth adding test coverage for this.

186–190

Rather than doing this, you should be able to just pass the NamedDecl * in to the call to Diag below and it will be properly formatted and quoted.

191

Please spell the type out in the declaration.

196–200

clang-tools-extra/docs/ReleaseNotes.rst

161

Can you re-flow this to 80 cols too?

clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp

262–263

I'm just trying to give back to the community after taking away a lot of reviewer effort with a humongous check that I just recently pushed in after years of (re-)development.

And your review efforts are very much seen and appreciated, @whisperity! Thank you for the excellent help!

[Sockke]

Sorry for the late reply. I made improvements to the patch in response to the questions you raised. @aaron.ballman

[Sockke]

clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp

182

Use InvocationParm to determine whether objects have a direct callee.

[aaron.ballman]

clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp

177–179

It looks like ReceivingExpr can be null (see line 78 that you added), so the first dyn_cast will crash if given null. Further, if the result of the first dynamic cast is null, then the second dyn_cast will also crash.

Should there be a null pointer check? Should we use cast instead of dyn_cast for the first one?

181

getDirectCallee() can return null.

185–191

getDirectCallee() can return nullptr.

[MTC]

This patch has become very complicated now. I summarized this patch and give a figure to illustrate what we have reached. And @Sockke please add some comments to explain the complex part or other means to make this patch more readable.

D107450.svg51 KBDownload

[Sockke]

Add some checks for null and comments for codes.

[Sockke]

clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp

161

ReceivingExpr is not null if IsRVRefParam is true.

165

Add a check for getDirectCallee().

175

I have added a check for getDirectCallee before here.

[Sockke]

Friendly ping. @aaron.ballman

[aaron.ballman]

LGTM! @Quuxplusone, you're still marked as requesting changes, are you okay with the way this has progressed?

[• Quuxplusone]

@Sockke: Throughout, trivially-copyable should be trivially copyable (two words).
Other than that, sure, I have no particular opinions at this point.

clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp

179

Throughout, Supress should be Suppress.

clang-tools-extra/docs/ReleaseNotes.rst

161

[Sockke]

Improve some description.

[aaron.ballman]

Do you need someone to commit on your behalf?

[Sockke]

In D107450#3257877, @aaron.ballman wrote:

Do you need someone to commit on your behalf?

Thanks for your review, I committed it myself.

clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.h

 //===--- MoveConstArgCheck.h - 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
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MOVECONSTANTARGUMENTCHECK_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MOVECONSTANTARGUMENTCHECK_H
 
 #include "../ClangTidyCheck.h"
+#include "llvm/ADT/DenseSet.h"
 
 namespace clang {
 namespace tidy {
 namespace performance {
 
 /// Find casts of calculation results to bigger type. Typically from int to
 ///
 /// There is one option:
bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
     return LangOpts.CPlusPlus;
   }
   void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
 
 private:
   const bool CheckTriviallyCopyableMove;
+  llvm::DenseSet<const CallExpr *> AlreadyCheckedMoves;
 };
 
 } // namespace performance
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MOVECONSTANTARGUMENTCHECK_H

clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp

 void MoveConstArgCheck::registerMatchers(MatchFinder *Finder) {
   auto MoveCallMatcher =
       callExpr(callee(functionDecl(hasName("::std::move"))), argumentCountIs(1),
                unless(isInTemplateInstantiation()))
           .bind("call-move");
 
   Finder->addMatcher(MoveCallMatcher, this);
 
+  auto ConstTypeParmMatcher =
+      qualType(references(isConstQualified())).bind("invocation-parm-type");
+  auto RValueTypeParmMatcher =
+      qualType(rValueReferenceType()).bind("invocation-parm-type");
+  // Matches respective ParmVarDecl for a CallExpr or CXXConstructExpr.
+  auto ArgumentWithParamMatcher = forEachArgumentWithParam(
+      MoveCallMatcher, parmVarDecl(anyOf(hasType(ConstTypeParmMatcher),
+                                         hasType(RValueTypeParmMatcher)))
+                           .bind("invocation-parm"));
+  // Matches respective types of arguments for a CallExpr or CXXConstructExpr
+  // and it works on calls through function pointers as well.
+  auto ArgumentWithParamTypeMatcher = forEachArgumentWithParamType(
+      MoveCallMatcher, anyOf(ConstTypeParmMatcher, RValueTypeParmMatcher));
+
   Finder->addMatcher(
-      invocation(forEachArgumentWithParam(
-                     MoveCallMatcher,
-                     parmVarDecl(hasType(references(isConstQualified())))))
+      invocation(anyOf(ArgumentWithParamMatcher, ArgumentWithParamTypeMatcher))
           .bind("receiving-expr"),
       this);
 }
 
+bool IsRValueReferenceParam(const Expr *Invocation,
+                            const QualType &InvocationParmType,
+                            const Expr *Arg) {
+  if (Invocation && InvocationParmType->isRValueReferenceType() &&
+      Arg->isLValue()) {
+    if (!Invocation->getType()->isRecordType())
+      return true;
+    else {
+      if (const auto *ConstructCallExpr =
+              dyn_cast<CXXConstructExpr>(Invocation)) {
+        if (const auto *ConstructorDecl = ConstructCallExpr->getConstructor()) {
+          if (!ConstructorDecl->isCopyOrMoveConstructor() &&
+              !ConstructorDecl->isDefaultConstructor())
+            return true;
+        }
+      }
+    }
+  }
+  return false;
+}
+
 void MoveConstArgCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *CallMove = Result.Nodes.getNodeAs<CallExpr>("call-move");
   const auto *ReceivingExpr = Result.Nodes.getNodeAs<Expr>("receiving-expr");
+  const auto *InvocationParm =
+      Result.Nodes.getNodeAs<ParmVarDecl>("invocation-parm");
+  const auto *InvocationParmType =
+      Result.Nodes.getNodeAs<QualType>("invocation-parm-type");
+
+  // Skipping matchers which have been matched.
+  if (!ReceivingExpr && AlreadyCheckedMoves.contains(CallMove))
+    return;
+
+  if (ReceivingExpr)
+    AlreadyCheckedMoves.insert(CallMove);
+
   const Expr *Arg = CallMove->getArg(0);
   SourceManager &SM = Result.Context->getSourceManager();
 
   CharSourceRange MoveRange =
       CharSourceRange::getCharRange(CallMove->getSourceRange());
   CharSourceRange FileMoveRange =
       Lexer::makeFileCharRange(MoveRange, SM, getLangOpts());
   if (!FileMoveRange.isValid())
if (const CXXRecordDecl *R = Arg->getType()->getAsCXXRecordDecl()) {
           return;
       }
     }
 
     if (!IsConstArg && IsTriviallyCopyable && !CheckTriviallyCopyableMove)
       return;
 
     bool IsVariable = isa<DeclRefExpr>(Arg);
+    // std::move shouldn't be removed when an lvalue wrapped by std::move is
+    // passed to the function with an rvalue reference parameter.
+    bool IsRVRefParam =
+        IsRValueReferenceParam(ReceivingExpr, *InvocationParmType, Arg);
     const auto *Var =
         IsVariable ? dyn_cast<DeclRefExpr>(Arg)->getDecl() : nullptr;
+
+    {
       auto Diag = diag(FileMoveRange.getBegin(),
                        "std::move of the %select{|const }0"
-                     "%select{expression|variable %4}1 "
-                     "%select{|of the trivially-copyable type %5 }2"
-                     "has no effect; remove std::move()"
-                     "%select{| or make the variable non-const}3")
+                       "%select{expression|variable %5}1 "
+                       "%select{|of the trivially-copyable type %6 }2"
+                       "has no effect%select{; remove std::move()|}3"
+                       "%select{| or make the variable non-const}4")
                   << IsConstArg << IsVariable << IsTriviallyCopyable
+                  << IsRVRefParam
                   << (IsConstArg && IsVariable && !IsTriviallyCopyable) << Var
                   << Arg->getType();
-
+      if (!IsRVRefParam)
         replaceCallWithArg(CallMove, Diag, SM, getLangOpts());
+    }
+    if (IsRVRefParam) {
+      // Generate notes for an invocation with an rvalue reference parameter.
+      const auto *ReceivingCallExpr = dyn_cast<CallExpr>(ReceivingExpr);
+      const auto *ReceivingConstructExpr =
+          dyn_cast<CXXConstructExpr>(ReceivingExpr);
+      // Skipping the invocation which is a template instantiation.
+      if ((!ReceivingCallExpr || !ReceivingCallExpr->getDirectCallee() ||
+           ReceivingCallExpr->getDirectCallee()->isTemplateInstantiation()) &&
+          (!ReceivingConstructExpr ||
+           !ReceivingConstructExpr->getConstructor() ||
+           ReceivingConstructExpr->getConstructor()->isTemplateInstantiation()))
+        return;
+
+      const NamedDecl *FunctionName = nullptr;
+      FunctionName =
+          ReceivingCallExpr
+              ? ReceivingCallExpr->getDirectCallee()->getUnderlyingDecl()
+              : ReceivingConstructExpr->getConstructor()->getUnderlyingDecl();
+
+      QualType NoRefType = (*InvocationParmType)->getPointeeType();
+      PrintingPolicy PolicyWithSuppressedTag(getLangOpts());
+      PolicyWithSuppressedTag.SuppressTagKeyword = true;
+      PolicyWithSuppressedTag.SuppressUnwrittenScope = true;
+      std::string ExpectParmTypeName =
+          NoRefType.getAsString(PolicyWithSuppressedTag);
+      if (!NoRefType->isPointerType()) {
+        NoRefType.addConst();
+        ExpectParmTypeName =
+            NoRefType.getAsString(PolicyWithSuppressedTag) + " &";
+      }
+
+      diag(InvocationParm->getLocation(),
+           "consider changing the %ordinal0 parameter of %1 from %2 to '%3'",
+           DiagnosticIDs::Note)
+          << (InvocationParm->getFunctionScopeIndex() + 1) << FunctionName
+          << *InvocationParmType << ExpectParmTypeName;
+    }
   } else if (ReceivingExpr) {
+    if ((*InvocationParmType)->isRValueReferenceType())
+      return;
+
     auto Diag = diag(FileMoveRange.getBegin(),
                      "passing result of std::move() as a const reference "
                      "argument; no move will actually happen");
 
     replaceCallWithArg(CallMove, Diag, SM, getLangOpts());
   }
 }
 
 } // namespace performance
 } // namespace tidy
 } // namespace clang

clang-tools-extra/docs/ReleaseNotes.rst

 - New alias :doc:`cert-flp37-c
   <clang-tidy/checks/cert-flp37-c>` to
   :doc:`bugprone-suspicious-memory-comparison
   <clang-tidy/checks/bugprone-suspicious-memory-comparison>` was added.
 
 Changes in existing checks
 ^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 - Removed default setting ``cppcoreguidelines-explicit-virtual-functions.IgnoreDestructors = "true"``,
   to match the current state of the C++ Core Guidelines.
 
 - Updated :doc:`google-readability-casting
   <clang-tidy/checks/google-readability-casting>` to diagnose and fix functional
   casts, to achieve feature parity with the corresponding ``cpplint.py`` check.
 
 - Fixed a false positive in :doc:`fuchsia-trailing-return
   <clang-tidy/checks/fuchsia-trailing-return>` for C++17 deduction guides.
 
 - Fixed a false positive in :doc:`bugprone-throw-keyword-missing
   <clang-tidy/checks/bugprone-throw-keyword-missing>` when creating an exception object
   using placement new
 
 - :doc:`cppcoreguidelines-narrowing-conversions <clang-tidy/checks/cppcoreguidelines-narrowing-conversions>`
   check now supports a `WarnOnIntegerToFloatingPointNarrowingConversion`
   option to control whether to warn on narrowing integer to floating-point
   conversions.
 
+- Improved :doc:`performance-move-const-arg` check.
+
+  Removed a wrong FixIt for trivially copyable objects wrapped by ``std::move()`` and passed to an rvalue reference parameter. Removal of ``std::move()`` would break the code.
 
 Removed checks
 ^^^^^^^^^^^^^^
 
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...

clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp

 int *f3(int *x3) {
   return std::move(x3);
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the variable 'x3' of the trivially-copyable type 'int *'
   // CHECK-FIXES: return x3;
 }
 
 A f4(A x4) { return std::move(x4); }
 
 A f5(const A x5) {
   return std::move(x5);
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the const variable 'x5' has no effect; remove std::move() or make the variable non-const [performance-move-const-arg]
   // CHECK-FIXES: return x5;
 }
 
 template <typename T>
 T f6(const T x6) {
   return std::move(x6);
void lambda2() {
 
   auto f = [callback = std::move(callback)](MoveSemantics m) mutable {
     // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: std::move of the variable 'callback' of the trivially-copyable type 'function<void (MoveSemantics)>' has no effect; remove std::move()
     // CHECK-FIXES: auto f = [callback = callback](MoveSemantics m) mutable {
     callback(std::move(m));
   };
   f(MoveSemantics());
 }
+
+void showInt(int &&v);
+void showInt(int v1, int &&v2);
+void showPointer(const char *&&s);
+void showPointer2(const char *const &&s);
+void showTriviallyCopyable(TriviallyCopyable &&obj);
+void showTriviallyCopyablePointer(const TriviallyCopyable *&&obj);
+void testFunctions() {
+  int a = 10;
+  showInt(std::move(a));
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-10]]:20: note: consider changing the 1st parameter of 'showInt' from 'int &&' to 'const int &'
+  showInt(int());
+  showInt(a, std::move(a));
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-13]]:28: note: consider changing the 2nd parameter of 'showInt' from 'int &&' to 'const int &'
+  const char* s = "";
+  showPointer(std::move(s));
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: std::move of the variable 's' of the trivially-copyable type 'const char *' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-16]]:32: note: consider changing the 1st parameter of 'showPointer' from 'const char *&&' to 'const char *'
+  showPointer2(std::move(s)); 
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: std::move of the variable 's' of the trivially-copyable type 'const char *' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-18]]:39: note: consider changing the 1st parameter of 'showPointer2' from 'const char *const &&' to 'const char *const'
+  TriviallyCopyable *obj = new TriviallyCopyable();
+  showTriviallyCopyable(std::move(*obj));
+  // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: std::move of the expression of the trivially-copyable type 'TriviallyCopyable' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-21]]:48: note: consider changing the 1st parameter of 'showTriviallyCopyable' from 'TriviallyCopyable &&' to 'const TriviallyCopyable &'
+  showTriviallyCopyablePointer(std::move(obj));
+  // CHECK-MESSAGES: :[[@LINE-1]]:32: warning: std::move of the variable 'obj' of the trivially-copyable type 'TriviallyCopyable *' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-23]]:62: note: consider changing the 1st parameter of 'showTriviallyCopyablePointer' from 'const TriviallyCopyable *&&' to 'const TriviallyCopyable *'
+}
+template <class T>
+void forwardToShowInt(T && t) {
+  showInt(static_cast<T &&>(t));
+}
+void testTemplate() {
+  int a = 10;
+  forwardToShowInt(std::move(a));
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect [performance-move-const-arg]
+}
+
+struct Tmp {
+  Tmp();
+  Tmp(int &&a);
+  Tmp(int v1, int &&a);
+  Tmp(const char *&&s);
+  Tmp(TriviallyCopyable&& obj);
+  Tmp(const TriviallyCopyable *&&obj);
+  void showTmp(TriviallyCopyable&& t);
+  static void showTmpStatic(TriviallyCopyable&& t);
+};
+void testMethods() {
+  Tmp t;
+  int a = 10;
+  Tmp t1(std::move(a));
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-13]]:13: note: consider changing the 1st parameter of 'Tmp' from 'int &&' to 'const int &'
+  Tmp t2(a, std::move(a));
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-15]]:21: note: consider changing the 2nd parameter of 'Tmp' from 'int &&' to 'const int &'
+  const char* s = "";
+  Tmp t3(std::move(s));
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the variable 's' of the trivially-copyable type 'const char *' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-18]]:21: note: consider changing the 1st parameter of 'Tmp' from 'const char *&&' to 'const char *'
+  TriviallyCopyable *obj = new TriviallyCopyable();
+  Tmp t4(std::move(*obj));
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the expression of the trivially-copyable type 'TriviallyCopyable' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-21]]:27: note: consider changing the 1st parameter of 'Tmp' from 'TriviallyCopyable &&' to 'const TriviallyCopyable &'
+  Tmp t5(std::move(obj));
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the variable 'obj' of the trivially-copyable type 'TriviallyCopyable *' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-23]]:34: note: consider changing the 1st parameter of 'Tmp' from 'const TriviallyCopyable *&&' to 'const TriviallyCopyable *'
+  t.showTmp(std::move(*obj));
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: std::move of the expression of the trivially-copyable type 'TriviallyCopyable' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-25]]:36: note: consider changing the 1st parameter of 'showTmp' from 'TriviallyCopyable &&' to 'const TriviallyCopyable &'
+  Tmp::showTmpStatic(std::move(*obj));
+  // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: std::move of the expression of the trivially-copyable type 'TriviallyCopyable' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-27]]:49: note: consider changing the 1st parameter of 'showTmpStatic' from 'TriviallyCopyable &&' to 'const TriviallyCopyable &'
+}
+
+void showA(A &&v) {}
+void testA() {
+  A a;
+  showA(std::move(a));
+}
+
+void testFuncPointer() {
+  int a = 10;
+  void (*choice)(int, int &&);
+  choice = showInt;
+  choice(std::move(a), std::move(a));
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect; remove std::move() [performance-move-const-arg]
+  // CHECK-FIXES: choice(a, std::move(a));
+  // CHECK-MESSAGES: :[[@LINE-3]]:24: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect [performance-move-const-arg]
+}