<div dir="ltr"><div>I see your point about how users who care should always be passing this check alongside "performance-move-const-arg"; but IMVHO it still makes sense for clang-tidy to warn unconditionally about code of the form</div><div><br></div><div> x = std::move(y);</div><div> use(y);</div><div><br></div><div>regardless of the incidental type of `y`. Sure, it's <i><b>technically</b></i> not wrong if `y` is const... or if `y` is a primitive type such as `Widget*`... or if `y` is a well-defined library type such as `std::shared_ptr<Widget>`... or if `y` is a library type that <a href="https://stackoverflow.com/questions/60175782/is-stdlistint-listreturn-stdmovelistlist-guaranteed-to-leave/60186200#comment106478458_60186200">works in practice, such as `std::list<int>`</a>... but regardless of its <i><b>technical</b></i> merits, the code is still <i><b>logically semantically</b></i> wrong, and I think that's what the clang-tidy check should be trying to diagnose.</div><div><br></div><div>my $.02,</div><div>Arthur</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sun, Feb 16, 2020 at 10:44 AM Zinovy Nis via Phabricator via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">zinovy.nis updated this revision to Diff 244878.<br>
<br>
CHANGES SINCE LAST ACTION<br>
<a href="https://reviews.llvm.org/D74692/new/" rel="noreferrer" target="_blank">https://reviews.llvm.org/D74692/new/</a><br>
<br>
<a href="https://reviews.llvm.org/D74692" rel="noreferrer" target="_blank">https://reviews.llvm.org/D74692</a><br>
<br>
Files:<br>
clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp<br>
clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp<br>
<br>
<br>
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp<br>
===================================================================<br>
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp<br>
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp<br>
@@ -129,6 +129,14 @@<br>
// CHECK-NOTES: [[@LINE-3]]:15: note: move occurred here<br>
}<br>
<br>
+// Simple case for const value: no actual move and no warning.<br>
+void simpleConst(const A a) {<br>
+ A other_a = std::move(a);<br>
+ a.foo();<br>
+ // CHECK-NOTES-NOT: [[@LINE-1]]:3: warning: 'a' used after it was moved<br>
+ // CHECK-NOTES-NOT: [[@LINE-3]]:15: note: move occurred here<br>
+}<br>
+<br>
// A warning should only be emitted for one use-after-move.<br>
void onlyFlagOneUseAfterMove() {<br>
A a;<br>
@@ -308,14 +316,15 @@<br>
}<br>
<br>
void lambdas() {<br>
- // Use-after-moves inside a lambda should be detected.<br>
+ // Use-after-moves inside a lambda will not be detected as [a]<br>
+ // is passed as a const by default.<br>
{<br>
A a;<br>
auto lambda = [a] {<br>
std::move(a);<br>
a.foo();<br>
- // CHECK-NOTES: [[@LINE-1]]:7: warning: 'a' used after it was moved<br>
- // CHECK-NOTES: [[@LINE-3]]:7: note: move occurred here<br>
+ // CHECK-NOTES-NOT: [[@LINE-1]]:7: warning: 'a' used after it was moved<br>
+ // CHECK-NOTES-NOT: [[@LINE-3]]:7: note: move occurred here<br>
};<br>
}<br>
// This is just as true if the variable was declared inside the lambda.<br>
@@ -720,15 +729,15 @@<br>
// const pointer argument.<br>
const A a;<br>
std::move(a);<br>
- passByConstPointer(&a);<br>
- // CHECK-NOTES: [[@LINE-1]]:25: warning: 'a' used after it was moved<br>
- // CHECK-NOTES: [[@LINE-3]]:5: note: move occurred here<br>
+ passByConstPointer(&a); // expect no warning: the const value was not moved.<br>
+ // CHECK-NOTES-NOT: [[@LINE-1]]:25: warning: 'a' used after it was moved<br>
+ // CHECK-NOTES-NOT: [[@LINE-3]]:5: note: move occurred here<br>
}<br>
const A a;<br>
std::move(a);<br>
- passByConstReference(a);<br>
- // CHECK-NOTES: [[@LINE-1]]:24: warning: 'a' used after it was moved<br>
- // CHECK-NOTES: [[@LINE-3]]:3: note: move occurred here<br>
+ passByConstReference(a); // expect no warning: the const value was not moved.<br>
+ // CHECK-NOTES-NOT: [[@LINE-1]]:24: warning: 'a' used after it was moved<br>
+ // CHECK-NOTES-NOT: [[@LINE-3]]:3: note: move occurred here<br>
}<br>
<br>
// Clearing a standard container using clear() is treated as a<br>
Index: clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp<br>
===================================================================<br>
--- clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp<br>
+++ clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp<br>
@@ -404,7 +404,8 @@<br>
hasArgument(0, declRefExpr().bind("arg")),<br>
anyOf(hasAncestor(lambdaExpr().bind("containing-lambda")),<br>
hasAncestor(functionDecl().bind("containing-func"))),<br>
- unless(inDecltypeOrTemplateArg()))<br>
+ unless(anyOf(inDecltypeOrTemplateArg(),<br>
+ hasType(qualType(isConstQualified())))))<br>
.bind("call-move");<br>
<br>
Finder->addMatcher(<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
</blockquote></div></div>