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 with multi-parameters, there is no problem, I will add them to the test file.
In D107450#2968708 <https://reviews.llvm.org/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? ================ Comment at: clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp:131-145 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()||; consider " + "changing %7's parameter from %8 to 'const %9 &'}3" + "%select{| or make the variable non-const}4") ---------------- whisperity wrote: > > ================ Comment at: clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp:139 << IsConstArg << IsVariable << IsTriviallyCopyable - << (IsConstArg && IsVariable && !IsTriviallyCopyable) << Var - << Arg->getType(); + << (IsRValueReferenceArg + (IsRValueReferenceArg && + IsTriviallyCopyable && ---------------- whisperity wrote: > **`+`**? 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. > **`+`**? 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. ================ Comment at: clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp:267-270 +void showTmp(Tmp &&) {} +void testTmp() { + Tmp t; + showTmp(std::move(t)); ---------------- whisperity wrote: > 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. The warning will only be given when calling this function and passing in the parameters wrapped with std::move. Is my understanding correct？ ================ Comment at: clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp:271 + showTmp(std::move(t)); + // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: std::move of the variable 't' of the trivially-copyable type 'Tmp' has no effect; consider changing showTmp's parameter from 'Tmp &&' to 'const Tmp &' +} ---------------- whisperity wrote: > We could ensure that the symbol name is printed the same way as the other > placeholder-injected named decls. Yes, this is a better implementation. ================ Comment at: clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp:261 + 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; consider changing showInt's parameter from 'int'&& to 'int'& + return std::move(a); ---------------- whisperity wrote: > MTC wrote: > > 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 > >>! 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. > >>! 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. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107450/new/ https://reviews.llvm.org/D107450 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
