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

Sockke added a comment. In D107450#3257877 , @aaron.ballman wrote: > Do you need someone to commit on your behalf? Thanks for your review, I committed it myself. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

This revision was automatically updated to reflect the committed changes. Closed by commit rGa7f8aea71485: [clang-tidy] Fix wrong FixIt in performance-move-const-arg (authored by Sockke). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107450/new/

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

aaron.ballman added a comment. Do you need someone to commit on your behalf? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107450/new/ https://reviews.llvm.org/D107450 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

Sockke updated this revision to Diff 401470. Sockke added a comment. Improve some description. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107450/new/ https://reviews.llvm.org/D107450 Files: clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp

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

Quuxplusone accepted this revision. Quuxplusone added a comment. This revision is now accepted and ready to land. @Sockke: Throughout, `trivially-copyable` should be `trivially copyable` (two words). Other than that, sure, I have no particular opinions at this point. Comment

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

aaron.ballman accepted this revision. aaron.ballman added a comment. LGTM! @Quuxplusone, you're still marked as requesting changes, are you okay with the way this has progressed? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107450/new/ https://reviews.llvm.org/D107450

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

Sockke added a comment. Friendly ping. @aaron.ballman CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107450/new/ https://reviews.llvm.org/D107450 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

Sockke added inline comments. Comment at: clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp:161 + // Generate notes for an invocation with an rvalue reference parameter. + const auto *ReceivingCallExpr = dyn_cast(ReceivingExpr); + const auto

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

Sockke updated this revision to Diff 398077. Sockke marked an inline comment as done. Sockke added a comment. Add some checks for `null` and comments for codes. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107450/new/ https://reviews.llvm.org/D107450 Files:

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

MTC added a comment. 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. F21496057: D107450.svg

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

aaron.ballman added inline comments. Comment at: clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp:149-151 + const auto *ReceivingCallExpr = dyn_cast(ReceivingExpr); + const auto *ReceivingConstructExpr = + dyn_cast(ReceivingExpr);

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

Sockke marked 13 inline comments as done. Sockke added inline comments. Comment at: clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp:145 + if ((!ReceivingCallExpr || + ReceivingCallExpr->getDirectCallee()->isTemplateInstantiation()) && +

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

Sockke updated this revision to Diff 396385. Sockke added a comment. Sorry for the late reply. I made improvements to the patch in response to the questions you raised. @aaron.ballman CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107450/new/ https://reviews.llvm.org/D107450 Files:

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

aaron.ballman added inline comments. Comment at: clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp:145 + if ((!ReceivingCallExpr || + ReceivingCallExpr->getDirectCallee()->isTemplateInstantiation()) && + (!ReceivingConstructExpr ||

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

Sockke added a comment. Kindly ping. @aaron.ballman CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107450/new/ https://reviews.llvm.org/D107450 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

Sockke added a comment. 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

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

whisperity added a comment. 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

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

Sockke added a comment. Hi, Could anyone please review this diff? @whisperity, @aaron.ballman CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107450/new/ https://reviews.llvm.org/D107450 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

Sockke added a comment. Kindly ping. @whisperity CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107450/new/ https://reviews.llvm.org/D107450 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

Sockke added a comment. Kindly ping. @whisperity CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107450/new/ https://reviews.llvm.org/D107450 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

Sockke updated this revision to Diff 384728. Sockke added a comment. In addition, the splicing logics of "FunctionName" and "ExpectParmTypeName" were improved. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107450/new/ https://reviews.llvm.org/D107450 Files:

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

Sockke updated this revision to Diff 384706. Sockke added a comment. I improved the logic of judgments needing to report suggesting notes in some cases. And more tests were added here. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107450/new/ https://reviews.llvm.org/D107450 Files:

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

whisperity added a comment. Okay. I think I am convinced. And removing a bogus automated fix is always a positive change! Comment at: clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp:144 + << (InvocationParm->getFunctionScopeIndex() + 1) + <<

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

Sockke marked 2 inline comments as done. Sockke added a comment. 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 >

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

Sockke updated this revision to Diff 382632. Sockke added a comment. Update! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107450/new/ https://reviews.llvm.org/D107450 Files: clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp

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

aaron.ballman added a comment. 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

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

whisperity added a comment. I think this is becoming okay and looks safe enough. I can't really grasp what `HasCheckedMoveSet` means, and how it synergises with storing `CallExpr`s so maybe a better name should be added. Do you mean `AlreadyCheckedMoves`? When is it possible that the same

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

whisperity added inline comments. Comment at: clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp:140 + diag(InvocationParm->getLocation(), + "consider changing the %0st parameter of %1 from %2 to 'const %3 &'", + DiagnosticIDs::Note)

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

Sockke added a comment. Herald added a subscriber: carlosgalvezp. Kindly ping. @whisperity, @aaron.ballman, @Quuxplusone CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107450/new/ https://reviews.llvm.org/D107450 ___ cfe-commits mailing list

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

Sockke added a comment. Hi, Could anyone please review this diff? @whisperity, @aaron.ballman CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107450/new/ https://reviews.llvm.org/D107450 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

Sockke marked 12 inline comments as done. Sockke added a comment. Kindly ping. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107450/new/ https://reviews.llvm.org/D107450 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

Sockke updated this revision to Diff 378168. Sockke retitled this revision from "[clang-tidy] Fix wrong FIxIt in performance-move-const-arg" to "[clang-tidy] Fix wrong FixIt in performance-move-const-arg". Sockke added a comment. Sorry for the delayed reply because of National Day. I have

[PATCH] D107450: [clang-tidy] Fix wrong FIxIt in performance-move-const-arg

whisperity added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp:254 + showInt(std::move(a)); + // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no

[PATCH] D107450: [clang-tidy] Fix wrong FIxIt in performance-move-const-arg

Sockke added a comment. Hi, Could you please take some time to review this diff again? @whisperity CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107450/new/ https://reviews.llvm.org/D107450 ___ cfe-commits mailing list

[PATCH] D107450: [clang-tidy] Fix wrong FIxIt in performance-move-const-arg

Sockke added a comment. kindly ping. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107450/new/ https://reviews.llvm.org/D107450 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D107450: [clang-tidy] Fix wrong FIxIt in performance-move-const-arg

Sockke marked an inline comment as not done. Sockke added a comment. 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

[PATCH] D107450: [clang-tidy] Fix wrong FIxIt in performance-move-const-arg

whisperity added a comment. 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,

[PATCH] D107450: [clang-tidy] Fix wrong FIxIt in performance-move-const-arg

Quuxplusone added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp:262-263 + int a = 10; + forwardToShowInt(std::move(a)); + // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: std::move of the variable 'a' of the

[PATCH] D107450: [clang-tidy] Fix wrong FIxIt in performance-move-const-arg

Sockke added a comment. Which do you prefer? @Quuxplusone Comment at: clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp:262-263 + int a = 10; + forwardToShowInt(std::move(a)); + // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: std::move of the variable

[PATCH] D107450: [clang-tidy] Fix wrong FIxIt in performance-move-const-arg

Quuxplusone added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp:262-263 + int a = 10; + forwardToShowInt(std::move(a)); + // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: std::move of the variable 'a' of the

[PATCH] D107450: [clang-tidy] Fix wrong FIxIt in performance-move-const-arg

Sockke added a comment. Any thoughts? : ) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107450/new/ https://reviews.llvm.org/D107450 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D107450: [clang-tidy] Fix wrong FIxIt in performance-move-const-arg

Sockke added a comment. Any thoughts? : ) Comment at: clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp:263 + forwardToShowInt(std::move(a)); + // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: std::move of the variable 'a' of the trivially-copyable type

[PATCH] D107450: [clang-tidy] Fix wrong FIxIt in performance-move-const-arg

Sockke updated this revision to Diff 367474. Sockke marked 2 inline comments as done. Sockke added a comment. Herald added a subscriber: jfb. Thanks for your reply @Quuxplusone. I have updated! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107450/new/ https://reviews.llvm.org/D107450

[PATCH] D107450: [clang-tidy] Fix wrong FIxIt in performance-move-const-arg

Quuxplusone added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp:263 + forwardToShowInt(std::move(a)); + // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: std::move of the variable 'a' of the trivially-copyable type 'int'

[PATCH] D107450: [clang-tidy] Fix wrong FIxIt in performance-move-const-arg

Sockke updated this revision to Diff 366815. Sockke added a comment. update! Fixed some diagnostic descriptions. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107450/new/ https://reviews.llvm.org/D107450 Files: clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp

[PATCH] D107450: [clang-tidy] Fix wrong FIxIt in performance-move-const-arg

Quuxplusone added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp:263 + forwardToShowInt(std::move(a)); + // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: std::move of the variable 'a' of the trivially-copyable type 'int'

[PATCH] D107450: [clang-tidy] Fix wrong FIxIt in performance-move-const-arg

Sockke updated this revision to Diff 366225. Sockke retitled this revision from "[clang-tidy] Fix wrong and missing warnings in performance-move-const-arg" to "[clang-tidy] Fix wrong FIxIt in performance-move-const-arg". Sockke edited the summary of this revision. Sockke added a comment. Fix