[PATCH] D74692: [clang-tidy][bugprone-use-after-move] Warn on std::move for consts

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

I personally don't think the note is useful. What we're trying to tell the user 
is "don't use X after you've moved out of X" -- whether the move actually has 
an effect or not is not useful information. To me, adding `; move of a 'const' 
argument has no effect` just makes things less clear -- are you trying to tell 
me I shouldn't use-after-move, or something else more subtle?

Just my .02


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74692/new/

https://reviews.llvm.org/D74692



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74692: [clang-tidy][bugprone-use-after-move] Warn on std::move for consts

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis added a comment.

One more gentle ping)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74692/new/

https://reviews.llvm.org/D74692



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74692: [clang-tidy][bugprone-use-after-move] Warn on std::move for consts

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis updated this revision to Diff 255128.
zinovy.nis added a comment.

Rebase over the current master.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74692/new/

https://reviews.llvm.org/D74692

Files:
  clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp
@@ -129,6 +129,16 @@
   // CHECK-NOTES: [[@LINE-3]]:15: note: move occurred here
 }
 
+void simpleConst() {
+  const A a;
+  a.foo();
+  A other_a = std::move(a);
+  a.foo();
+  // CHECK-NOTES: [[@LINE-1]]:3: warning: 'a' used after it was moved; move of a 'const' argument has no effect
+  // CHECK-NOTES: [[@LINE-3]]:15: note: move occurred here
+  // CHECK-NOTES: [[@LINE-6]]:11: note: variable 'a' declared const here
+}
+
 // A warning should only be emitted for one use-after-move.
 void onlyFlagOneUseAfterMove() {
   A a;
@@ -314,8 +324,21 @@
 auto lambda = [a] {
   std::move(a);
   a.foo();
-  // CHECK-NOTES: [[@LINE-1]]:7: warning: 'a' used after it was moved
+  // CHECK-NOTES: [[@LINE-1]]:7: warning: 'a' used after it was moved; move of a 'const' argument has no effect
   // CHECK-NOTES: [[@LINE-3]]:7: note: move occurred here
+  // CHECK-NOTES: [[@LINE-5]]:20: note: variable 'a' captured const here
+};
+  }
+  // Use-after-moves inside a lambda should be detected.
+  {
+A a;
+// Implicit capture
+auto lambda = [=] {
+  std::move(a);
+  a.foo();
+  // CHECK-NOTES: [[@LINE-1]]:7: warning: 'a' used after it was moved; move of a 'const' argument has no effect
+  // CHECK-NOTES: [[@LINE-3]]:7: note: move occurred here
+  // CHECK-NOTES: [[@LINE-5]]:20: note: variable 'a' implicitly captured const here
 };
   }
   // This is just as true if the variable was declared inside the lambda.
@@ -721,14 +744,16 @@
 const A a;
 std::move(a);
 passByConstPointer();
-// CHECK-NOTES: [[@LINE-1]]:25: warning: 'a' used after it was moved
+// CHECK-NOTES: [[@LINE-1]]:25: warning: 'a' used after it was moved; move of a 'const' argument has no effect
 // CHECK-NOTES: [[@LINE-3]]:5: note: move occurred here
+// CHECK-NOTES: [[@LINE-5]]:13: note: variable 'a' declared const here
   }
   const A a;
   std::move(a);
   passByConstReference(a);
-  // CHECK-NOTES: [[@LINE-1]]:24: warning: 'a' used after it was moved
+  // CHECK-NOTES: [[@LINE-1]]:24: warning: 'a' used after it was moved; move of a 'const' argument has no effect
   // CHECK-NOTES: [[@LINE-3]]:3: note: move occurred here
+  // CHECK-NOTES: [[@LINE-5]]:11: note: variable 'a' declared const here
 }
 
 // Clearing a standard container using clear() is treated as a
Index: clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
@@ -373,14 +373,36 @@
 }
 
 static void emitDiagnostic(const Expr *MovingCall, const DeclRefExpr *MoveArg,
-   const UseAfterMove , ClangTidyCheck *Check,
+   const UseAfterMove ,
+   const LambdaExpr *Lambda, ClangTidyCheck *Check,
ASTContext *Context) {
   SourceLocation UseLoc = Use.DeclRef->getExprLoc();
   SourceLocation MoveLoc = MovingCall->getExprLoc();
+  int MoveArgIsConst = MoveArg->getType().isConstQualified();
 
-  Check->diag(UseLoc, "'%0' used after it was moved")
-  << MoveArg->getDecl()->getName();
+  Check->diag(UseLoc, "'%0' used after it was moved%select{|; move of a "
+  "'const' argument has no effect}1")
+  << MoveArg->getDecl()->getName() << MoveArgIsConst;
   Check->diag(MoveLoc, "move occurred here", DiagnosticIDs::Note);
+  if (MoveArgIsConst) {
+if (Lambda) {
+  for (const auto  : Lambda->captures()) {
+if (MoveArg->getDecl() == Capture.getCapturedVar()) {
+  int IsExplicitCapture = Capture.isExplicit();
+  Check->diag(IsExplicitCapture ? Capture.getLocation()
+: Lambda->getCaptureDefaultLoc(),
+  "variable %0 %select{implicitly|}1 captured const here",
+  DiagnosticIDs::Note)
+  << Capture.getCapturedVar() << IsExplicitCapture;
+  break;
+}
+  }
+} else {
+  Check->diag(MoveArg->getDecl()->getLocation(),
+  "variable '%0' declared const here", DiagnosticIDs::Note);
+}
+  }
+
   if (Use.EvaluationOrderUndefined) {
 Check->diag(UseLoc,
 "the use and move are unsequenced, i.e. 

[PATCH] D74692: [clang-tidy][bugprone-use-after-move] Warn on std::move for consts

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis added a comment.

Gentle ping)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74692/new/

https://reviews.llvm.org/D74692



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74692: [clang-tidy][bugprone-use-after-move] Warn on std::move for consts

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added reviewers: lebedev.ri, JonasToth, ldionne.
aaron.ballman added a comment.
Herald added a subscriber: dexonsmith.

In D74692#1923315 , @zinovy.nis wrote:

> In D74692#1923191 , @Quuxplusone 
> wrote:
>
> > I still think this entire patch is misguided; there's no reason to make the 
> > note for `const std::string s; std::move(s)` any longer than the note for 
> > `int i; std::move(i)` or `volatile std::string v; std::move(v)`. People 
> > should not be using moved-from objects, period; and those who want to use 
> > moved-from objects, should not enable this clang-tidy check.
> >
> > However, I have no further comments //besides// philosophical opposition to 
> > the whole idea.
>
>
> By this patch I'd like to provide more helpful info to the user on why the 
> code is wrong.
>  Anyway I don't like to submit this patch if you still have such strong 
> objections.


I don't feel strongly in favor of this patch, but I also am not strongly 
opposed to it. I've added some more reviewers to see if there are other 
opinions on the matter.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74692/new/

https://reviews.llvm.org/D74692



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74692: [clang-tidy][bugprone-use-after-move] Warn on std::move for consts

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis added a comment.

In D74692#1923191 , @Quuxplusone wrote:

> I still think this entire patch is misguided; there's no reason to make the 
> note for `const std::string s; std::move(s)` any longer than the note for 
> `int i; std::move(i)` or `volatile std::string v; std::move(v)`. People 
> should not be using moved-from objects, period; and those who want to use 
> moved-from objects, should not enable this clang-tidy check.
>
> However, I have no further comments //besides// philosophical opposition to 
> the whole idea.


By this patch I'd like to provide more helpful info to the user on why the code 
is wrong.
Anyway I don't like submit this patch if you still have objections.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74692/new/

https://reviews.llvm.org/D74692



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74692: [clang-tidy][bugprone-use-after-move] Warn on std::move for consts

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added a comment.

I still think this entire patch is misguided; there's no reason to make the 
note for `const std::string s; std::move(s)` any longer than the note for `int 
i; std::move(i)` or `volatile std::string v; std::move(v)`. People should not 
be using moved-from objects, period; and those who want to use moved-from 
objects, should not enable this clang-tidy check.

However, I have no further comments //besides// philosophical opposition to the 
whole idea.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74692/new/

https://reviews.llvm.org/D74692



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74692: [clang-tidy][bugprone-use-after-move] Warn on std::move for consts

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis updated this revision to Diff 250375.
zinovy.nis added a comment.

Removed top-level consts.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74692/new/

https://reviews.llvm.org/D74692

Files:
  clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp
@@ -129,6 +129,16 @@
   // CHECK-NOTES: [[@LINE-3]]:15: note: move occurred here
 }
 
+void simpleConst() {
+  const A a;
+  a.foo();
+  A other_a = std::move(a);
+  a.foo();
+  // CHECK-NOTES: [[@LINE-1]]:3: warning: 'a' used after it was moved; move of a 'const' argument has no effect
+  // CHECK-NOTES: [[@LINE-3]]:15: note: move occurred here
+  // CHECK-NOTES: [[@LINE-6]]:11: note: variable 'a' declared const here
+}
+
 // A warning should only be emitted for one use-after-move.
 void onlyFlagOneUseAfterMove() {
   A a;
@@ -314,8 +324,21 @@
 auto lambda = [a] {
   std::move(a);
   a.foo();
-  // CHECK-NOTES: [[@LINE-1]]:7: warning: 'a' used after it was moved
+  // CHECK-NOTES: [[@LINE-1]]:7: warning: 'a' used after it was moved; move of a 'const' argument has no effect
   // CHECK-NOTES: [[@LINE-3]]:7: note: move occurred here
+  // CHECK-NOTES: [[@LINE-5]]:20: note: variable 'a' captured const here
+};
+  }
+  // Use-after-moves inside a lambda should be detected.
+  {
+A a;
+// Implicit capture
+auto lambda = [=] {
+  std::move(a);
+  a.foo();
+  // CHECK-NOTES: [[@LINE-1]]:7: warning: 'a' used after it was moved; move of a 'const' argument has no effect
+  // CHECK-NOTES: [[@LINE-3]]:7: note: move occurred here
+  // CHECK-NOTES: [[@LINE-5]]:20: note: variable 'a' implicitly captured const here
 };
   }
   // This is just as true if the variable was declared inside the lambda.
@@ -721,14 +744,16 @@
 const A a;
 std::move(a);
 passByConstPointer();
-// CHECK-NOTES: [[@LINE-1]]:25: warning: 'a' used after it was moved
+// CHECK-NOTES: [[@LINE-1]]:25: warning: 'a' used after it was moved; move of a 'const' argument has no effect
 // CHECK-NOTES: [[@LINE-3]]:5: note: move occurred here
+// CHECK-NOTES: [[@LINE-5]]:13: note: variable 'a' declared const here
   }
   const A a;
   std::move(a);
   passByConstReference(a);
-  // CHECK-NOTES: [[@LINE-1]]:24: warning: 'a' used after it was moved
+  // CHECK-NOTES: [[@LINE-1]]:24: warning: 'a' used after it was moved; move of a 'const' argument has no effect
   // CHECK-NOTES: [[@LINE-3]]:3: note: move occurred here
+  // CHECK-NOTES: [[@LINE-5]]:11: note: variable 'a' declared const here
 }
 
 // Clearing a standard container using clear() is treated as a
Index: clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
@@ -373,14 +373,36 @@
 }
 
 static void emitDiagnostic(const Expr *MovingCall, const DeclRefExpr *MoveArg,
-   const UseAfterMove , ClangTidyCheck *Check,
+   const UseAfterMove ,
+   const LambdaExpr *Lambda, ClangTidyCheck *Check,
ASTContext *Context) {
   SourceLocation UseLoc = Use.DeclRef->getExprLoc();
   SourceLocation MoveLoc = MovingCall->getExprLoc();
+  int MoveArgIsConst = MoveArg->getType().isConstQualified();
 
-  Check->diag(UseLoc, "'%0' used after it was moved")
-  << MoveArg->getDecl()->getName();
+  Check->diag(UseLoc, "'%0' used after it was moved%select{|; move of a "
+  "'const' argument has no effect}1")
+  << MoveArg->getDecl()->getName() << MoveArgIsConst;
   Check->diag(MoveLoc, "move occurred here", DiagnosticIDs::Note);
+  if (MoveArgIsConst) {
+if (Lambda) {
+  for (const auto  : Lambda->captures()) {
+if (MoveArg->getDecl() == Capture.getCapturedVar()) {
+  int IsExplicitCapture = Capture.isExplicit();
+  Check->diag(IsExplicitCapture ? Capture.getLocation()
+: Lambda->getCaptureDefaultLoc(),
+  "variable %0 %select{implicitly|}1 captured const here",
+  DiagnosticIDs::Note)
+  << Capture.getCapturedVar() << IsExplicitCapture;
+  break;
+}
+  }
+} else {
+  Check->diag(MoveArg->getDecl()->getLocation(),
+  "variable '%0' declared const here", DiagnosticIDs::Note);
+}
+  }
+
   if (Use.EvaluationOrderUndefined) {
 Check->diag(UseLoc,
 "the use and move are unsequenced, i.e. there 

[PATCH] D74692: [clang-tidy][bugprone-use-after-move] Warn on std::move for consts

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM aside from the `const` nits. You should wait for @Quuxplusone before 
committing in case there are any additional comments.




Comment at: clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp:383
   SourceLocation MoveLoc = MovingCall->getExprLoc();
+  const bool MoveArgIsConst = MoveArg->getType().isConstQualified();
 

aaron.ballman wrote:
> Drop the top-level `const` and make it an integer type so that you don't need 
> to use the `?:` below. The implicit conversion will do the right thing in 
> terms of the integer value.
Top-level `const` is still here on the declaration.



Comment at: clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp:393
+if (MoveArg->getDecl() == Capture.getCapturedVar()) {
+  const int IsExplicitCapture = Capture.isExplicit();
+  Check->diag(IsExplicitCapture ? Capture.getLocation()

Top-level `const` is still here as well.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74692/new/

https://reviews.llvm.org/D74692



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74692: [clang-tidy][bugprone-use-after-move] Warn on std::move for consts

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis added a comment.

I'm done, Aaron, Quuxplusone, do you have any comments?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74692/new/

https://reviews.llvm.org/D74692



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74692: [clang-tidy][bugprone-use-after-move] Warn on std::move for consts

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis marked 2 inline comments as done.
zinovy.nis added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp:342
+  // CHECK-NOTES: [[@LINE-5]]:20: note: variable 'a' implicitly captured 
const here
 };
   }

Quuxplusone wrote:
> zinovy.nis wrote:
> > aaron.ballman wrote:
> > > One more test case to try out (it might be a FIXME because I imagine this 
> > > requires flow control to get right):
> > > ```
> > > A a;
> > > std::move(a);
> > > 
> > > auto lambda = [=] {
> > >   a.foo(); // Use of 'a' after it was moved
> > > }
> > > ```
> > `a` in lambda is `const`, but it's not moved inside lambda, so my warning 
> > is not expected to be shown.
> The "use" of `a` that Aaron was talking about is actually
> 
> auto lambda = [=] { a.foo(); };
>^ here
> 
> where the moved-from object is captured-by-copy into the lambda. Making a 
> copy of a moved-from object should warn. (Your patch shouldn't have affected 
> this AFAICT; I think he's just asking for a test to verify+document the 
> currently expected behavior.)
https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp#L342

Seems these tests have already been added.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74692/new/

https://reviews.llvm.org/D74692



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74692: [clang-tidy][bugprone-use-after-move] Warn on std::move for consts

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis added a comment.

In D74692#1911150 , @Quuxplusone wrote:

> Anyway, I still don't see the point of this patch. It seems like you're just 
> duplicating the work of `performance-move-const-arg`. People who want to be 
> shown notes about moves-of-const-args should just enable that check instead.


Well, I understand your point. But moving a const is NOP so `bugprone-*` should 
not be warned at all?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74692/new/

https://reviews.llvm.org/D74692



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74692: [clang-tidy][bugprone-use-after-move] Warn on std::move for consts

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added a comment.

Anyway, I still don't see the point of this patch. It seems like you're just 
duplicating the work of `performance-move-const-arg`. People who want to be 
shown notes about moves-of-const-args should just enable that check instead.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74692/new/

https://reviews.llvm.org/D74692



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74692: [clang-tidy][bugprone-use-after-move] Warn on std::move for consts

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp:342
+  // CHECK-NOTES: [[@LINE-5]]:20: note: variable 'a' implicitly captured 
const here
 };
   }

zinovy.nis wrote:
> aaron.ballman wrote:
> > One more test case to try out (it might be a FIXME because I imagine this 
> > requires flow control to get right):
> > ```
> > A a;
> > std::move(a);
> > 
> > auto lambda = [=] {
> >   a.foo(); // Use of 'a' after it was moved
> > }
> > ```
> `a` in lambda is `const`, but it's not moved inside lambda, so my warning is 
> not expected to be shown.
The "use" of `a` that Aaron was talking about is actually

auto lambda = [=] { a.foo(); };
   ^ here

where the moved-from object is captured-by-copy into the lambda. Making a copy 
of a moved-from object should warn. (Your patch shouldn't have affected this 
AFAICT; I think he's just asking for a test to verify+document the currently 
expected behavior.)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74692/new/

https://reviews.llvm.org/D74692



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74692: [clang-tidy][bugprone-use-after-move] Warn on std::move for consts

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp:342
+  // CHECK-NOTES: [[@LINE-5]]:20: note: variable 'a' implicitly captured 
const here
 };
   }

aaron.ballman wrote:
> One more test case to try out (it might be a FIXME because I imagine this 
> requires flow control to get right):
> ```
> A a;
> std::move(a);
> 
> auto lambda = [=] {
>   a.foo(); // Use of 'a' after it was moved
> }
> ```
`a` in lambda is `const`, but it's not moved inside lambda, so my warning is 
not expected to be shown.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74692/new/

https://reviews.llvm.org/D74692



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74692: [clang-tidy][bugprone-use-after-move] Warn on std::move for consts

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis updated this revision to Diff 248905.
zinovy.nis marked 4 inline comments as done.
zinovy.nis added a comment.

Cosmetic and style fixes.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74692/new/

https://reviews.llvm.org/D74692

Files:
  clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp
@@ -129,6 +129,16 @@
   // CHECK-NOTES: [[@LINE-3]]:15: note: move occurred here
 }
 
+void simpleConst() {
+  const A a;
+  a.foo();
+  A other_a = std::move(a);
+  a.foo();
+  // CHECK-NOTES: [[@LINE-1]]:3: warning: 'a' used after it was moved; move of a 'const' argument has no effect
+  // CHECK-NOTES: [[@LINE-3]]:15: note: move occurred here
+  // CHECK-NOTES: [[@LINE-6]]:11: note: variable 'a' declared const here
+}
+
 // A warning should only be emitted for one use-after-move.
 void onlyFlagOneUseAfterMove() {
   A a;
@@ -314,8 +324,21 @@
 auto lambda = [a] {
   std::move(a);
   a.foo();
-  // CHECK-NOTES: [[@LINE-1]]:7: warning: 'a' used after it was moved
+  // CHECK-NOTES: [[@LINE-1]]:7: warning: 'a' used after it was moved; move of a 'const' argument has no effect
   // CHECK-NOTES: [[@LINE-3]]:7: note: move occurred here
+  // CHECK-NOTES: [[@LINE-5]]:20: note: variable 'a' captured const here
+};
+  }
+  // Use-after-moves inside a lambda should be detected.
+  {
+A a;
+// Implicit capture
+auto lambda = [=] {
+  std::move(a);
+  a.foo();
+  // CHECK-NOTES: [[@LINE-1]]:7: warning: 'a' used after it was moved; move of a 'const' argument has no effect
+  // CHECK-NOTES: [[@LINE-3]]:7: note: move occurred here
+  // CHECK-NOTES: [[@LINE-5]]:20: note: variable 'a' implicitly captured const here
 };
   }
   // This is just as true if the variable was declared inside the lambda.
@@ -721,14 +744,16 @@
 const A a;
 std::move(a);
 passByConstPointer();
-// CHECK-NOTES: [[@LINE-1]]:25: warning: 'a' used after it was moved
+// CHECK-NOTES: [[@LINE-1]]:25: warning: 'a' used after it was moved; move of a 'const' argument has no effect
 // CHECK-NOTES: [[@LINE-3]]:5: note: move occurred here
+// CHECK-NOTES: [[@LINE-5]]:13: note: variable 'a' declared const here
   }
   const A a;
   std::move(a);
   passByConstReference(a);
-  // CHECK-NOTES: [[@LINE-1]]:24: warning: 'a' used after it was moved
+  // CHECK-NOTES: [[@LINE-1]]:24: warning: 'a' used after it was moved; move of a 'const' argument has no effect
   // CHECK-NOTES: [[@LINE-3]]:3: note: move occurred here
+  // CHECK-NOTES: [[@LINE-5]]:11: note: variable 'a' declared const here
 }
 
 // Clearing a standard container using clear() is treated as a
Index: clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
@@ -375,14 +375,36 @@
 }
 
 static void emitDiagnostic(const Expr *MovingCall, const DeclRefExpr *MoveArg,
-   const UseAfterMove , ClangTidyCheck *Check,
+   const UseAfterMove ,
+   const LambdaExpr *Lambda, ClangTidyCheck *Check,
ASTContext *Context) {
   SourceLocation UseLoc = Use.DeclRef->getExprLoc();
   SourceLocation MoveLoc = MovingCall->getExprLoc();
+  const int MoveArgIsConst = MoveArg->getType().isConstQualified();
 
-  Check->diag(UseLoc, "'%0' used after it was moved")
-  << MoveArg->getDecl()->getName();
+  Check->diag(UseLoc, "'%0' used after it was moved%select{|; move of a "
+  "'const' argument has no effect}1")
+  << MoveArg->getDecl()->getName() << MoveArgIsConst;
   Check->diag(MoveLoc, "move occurred here", DiagnosticIDs::Note);
+  if (MoveArgIsConst) {
+if (Lambda) {
+  for (const auto  : Lambda->captures()) {
+if (MoveArg->getDecl() == Capture.getCapturedVar()) {
+  const int IsExplicitCapture = Capture.isExplicit();
+  Check->diag(IsExplicitCapture ? Capture.getLocation()
+: Lambda->getCaptureDefaultLoc(),
+  "variable %0 %select{implicitly|}1 captured const here",
+  DiagnosticIDs::Note)
+  << Capture.getCapturedVar() << IsExplicitCapture;
+  break;
+}
+  }
+} else {
+  Check->diag(MoveArg->getDecl()->getLocation(),
+  "variable '%0' declared const here", DiagnosticIDs::Note);
+}
+  }
+
   if (Use.EvaluationOrderUndefined) {
 Check->diag(UseLoc,
   

[PATCH] D74692: [clang-tidy][bugprone-use-after-move] Warn on std::move for consts

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp:383
   SourceLocation MoveLoc = MovingCall->getExprLoc();
+  const bool MoveArgIsConst = MoveArg->getType().isConstQualified();
 

Drop the top-level `const` and make it an integer type so that you don't need 
to use the `?:` below. The implicit conversion will do the right thing in terms 
of the integer value.



Comment at: clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp:391-392
+if (Lambda) {
+  for (auto Capture = Lambda->capture_begin(), End = Lambda->capture_end();
+   Capture != End; ++Capture) {
+if (MoveArg->getDecl() == Capture->getCapturedVar()) {

Range-based for loop over `captures()`?



Comment at: clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp:394
+if (MoveArg->getDecl() == Capture->getCapturedVar()) {
+  const bool IsExplicitCapture = Capture->isExplicit();
+  Check->diag(

Same suggestions here as above.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp:342
+  // CHECK-NOTES: [[@LINE-5]]:20: note: variable 'a' implicitly captured 
const here
 };
   }

One more test case to try out (it might be a FIXME because I imagine this 
requires flow control to get right):
```
A a;
std::move(a);

auto lambda = [=] {
  a.foo(); // Use of 'a' after it was moved
}
```


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74692/new/

https://reviews.llvm.org/D74692



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74692: [clang-tidy][bugprone-use-after-move] Warn on std::move for consts

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis added a comment.

Any further comments?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74692/new/

https://reviews.llvm.org/D74692



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74692: [clang-tidy][bugprone-use-after-move] Warn on std::move for consts

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis updated this revision to Diff 247985.
zinovy.nis added a comment.

Typo fixed.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74692/new/

https://reviews.llvm.org/D74692

Files:
  clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp
@@ -129,6 +129,16 @@
   // CHECK-NOTES: [[@LINE-3]]:15: note: move occurred here
 }
 
+void simpleConst() {
+  const A a;
+  a.foo();
+  A other_a = std::move(a);
+  a.foo();
+  // CHECK-NOTES: [[@LINE-1]]:3: warning: 'a' used after it was moved; move of a 'const' argument has no effect
+  // CHECK-NOTES: [[@LINE-3]]:15: note: move occurred here
+  // CHECK-NOTES: [[@LINE-6]]:11: note: variable 'a' declared const here
+}
+
 // A warning should only be emitted for one use-after-move.
 void onlyFlagOneUseAfterMove() {
   A a;
@@ -314,8 +324,21 @@
 auto lambda = [a] {
   std::move(a);
   a.foo();
-  // CHECK-NOTES: [[@LINE-1]]:7: warning: 'a' used after it was moved
+  // CHECK-NOTES: [[@LINE-1]]:7: warning: 'a' used after it was moved; move of a 'const' argument has no effect
   // CHECK-NOTES: [[@LINE-3]]:7: note: move occurred here
+  // CHECK-NOTES: [[@LINE-5]]:20: note: variable 'a' captured const here
+};
+  }
+  // Use-after-moves inside a lambda should be detected.
+  {
+A a;
+// Implicit capture
+auto lambda = [=] {
+  std::move(a);
+  a.foo();
+  // CHECK-NOTES: [[@LINE-1]]:7: warning: 'a' used after it was moved; move of a 'const' argument has no effect
+  // CHECK-NOTES: [[@LINE-3]]:7: note: move occurred here
+  // CHECK-NOTES: [[@LINE-5]]:20: note: variable 'a' implicitly captured const here
 };
   }
   // This is just as true if the variable was declared inside the lambda.
@@ -721,14 +744,16 @@
 const A a;
 std::move(a);
 passByConstPointer();
-// CHECK-NOTES: [[@LINE-1]]:25: warning: 'a' used after it was moved
+// CHECK-NOTES: [[@LINE-1]]:25: warning: 'a' used after it was moved; move of a 'const' argument has no effect
 // CHECK-NOTES: [[@LINE-3]]:5: note: move occurred here
+// CHECK-NOTES: [[@LINE-5]]:13: note: variable 'a' declared const here
   }
   const A a;
   std::move(a);
   passByConstReference(a);
-  // CHECK-NOTES: [[@LINE-1]]:24: warning: 'a' used after it was moved
+  // CHECK-NOTES: [[@LINE-1]]:24: warning: 'a' used after it was moved; move of a 'const' argument has no effect
   // CHECK-NOTES: [[@LINE-3]]:3: note: move occurred here
+  // CHECK-NOTES: [[@LINE-5]]:11: note: variable 'a' declared const here
 }
 
 // Clearing a standard container using clear() is treated as a
Index: clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
@@ -375,14 +375,38 @@
 }
 
 static void emitDiagnostic(const Expr *MovingCall, const DeclRefExpr *MoveArg,
-   const UseAfterMove , ClangTidyCheck *Check,
+   const UseAfterMove ,
+   const LambdaExpr *Lambda, ClangTidyCheck *Check,
ASTContext *Context) {
   SourceLocation UseLoc = Use.DeclRef->getExprLoc();
   SourceLocation MoveLoc = MovingCall->getExprLoc();
+  const bool MoveArgIsConst = MoveArg->getType().isConstQualified();
 
-  Check->diag(UseLoc, "'%0' used after it was moved")
-  << MoveArg->getDecl()->getName();
+  Check->diag(UseLoc, "'%0' used after it was moved%select{|; move of a 'const' argument has no effect}1")
+  << MoveArg->getDecl()->getName()
+  << (MoveArgIsConst ? 1 : 0);
   Check->diag(MoveLoc, "move occurred here", DiagnosticIDs::Note);
+  if (MoveArgIsConst) {
+if (Lambda) {
+  for (auto Capture = Lambda->capture_begin(), End = Lambda->capture_end();
+   Capture != End; ++Capture) {
+if (MoveArg->getDecl() == Capture->getCapturedVar()) {
+  const bool IsExplicitCapture = Capture->isExplicit();
+  Check->diag(
+  IsExplicitCapture ? Capture->getLocation()
+: Lambda->getCaptureDefaultLoc(),
+  "variable %0 %select{|implicitly}1 captured const here",
+  DiagnosticIDs::Note)
+  << Capture->getCapturedVar() << (IsExplicitCapture ? 0 : 1);
+  break;
+}
+  }
+} else {
+  Check->diag(MoveArg->getDecl()->getLocation(),
+  "variable '%0' declared const here", DiagnosticIDs::Note);
+}
+  }
+
   if (Use.EvaluationOrderUndefined) {
 

[PATCH] D74692: [clang-tidy][bugprone-use-after-move] Warn on std::move for consts

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis updated this revision to Diff 247982.
zinovy.nis added a comment.

1. Special handling for captured variables in lambdas,
2. messages texts were changed.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74692/new/

https://reviews.llvm.org/D74692

Files:
  clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp
@@ -129,6 +129,16 @@
   // CHECK-NOTES: [[@LINE-3]]:15: note: move occurred here
 }
 
+void simpleConst() {
+  const A a;
+  a.foo();
+  A other_a = std::move(a);
+  a.foo();
+  // CHECK-NOTES: [[@LINE-1]]:3: warning: 'a' used after it was moved; move of a 'const' argument has no effect
+  // CHECK-NOTES: [[@LINE-3]]:15: note: move occurred here
+  // CHECK-NOTES: [[@LINE-6]]:11: note: variable 'a' declared as const here
+}
+
 // A warning should only be emitted for one use-after-move.
 void onlyFlagOneUseAfterMove() {
   A a;
@@ -314,8 +324,21 @@
 auto lambda = [a] {
   std::move(a);
   a.foo();
-  // CHECK-NOTES: [[@LINE-1]]:7: warning: 'a' used after it was moved
+  // CHECK-NOTES: [[@LINE-1]]:7: warning: 'a' used after it was moved; move of a 'const' argument has no effect
   // CHECK-NOTES: [[@LINE-3]]:7: note: move occurred here
+  // CHECK-NOTES: [[@LINE-5]]:20: note: variable 'a' captured as const here
+};
+  }
+  // Use-after-moves inside a lambda should be detected.
+  {
+A a;
+// Implicit capture
+auto lambda = [=] {
+  std::move(a);
+  a.foo();
+  // CHECK-NOTES: [[@LINE-1]]:7: warning: 'a' used after it was moved; move of a 'const' argument has no effect
+  // CHECK-NOTES: [[@LINE-3]]:7: note: move occurred here
+  // CHECK-NOTES: [[@LINE-5]]:20: note: variable 'a' implicitly captured as const here
 };
   }
   // This is just as true if the variable was declared inside the lambda.
@@ -721,14 +744,16 @@
 const A a;
 std::move(a);
 passByConstPointer();
-// CHECK-NOTES: [[@LINE-1]]:25: warning: 'a' used after it was moved
+// CHECK-NOTES: [[@LINE-1]]:25: warning: 'a' used after it was moved; move of a 'const' argument has no effect
 // CHECK-NOTES: [[@LINE-3]]:5: note: move occurred here
+// CHECK-NOTES: [[@LINE-5]]:13: note: variable 'a' declared as const here
   }
   const A a;
   std::move(a);
   passByConstReference(a);
-  // CHECK-NOTES: [[@LINE-1]]:24: warning: 'a' used after it was moved
+  // CHECK-NOTES: [[@LINE-1]]:24: warning: 'a' used after it was moved; move of a 'const' argument has no effect
   // CHECK-NOTES: [[@LINE-3]]:3: note: move occurred here
+  // CHECK-NOTES: [[@LINE-5]]:11: note: variable 'a' declared as const here
 }
 
 // Clearing a standard container using clear() is treated as a
Index: clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
@@ -375,14 +375,38 @@
 }
 
 static void emitDiagnostic(const Expr *MovingCall, const DeclRefExpr *MoveArg,
-   const UseAfterMove , ClangTidyCheck *Check,
+   const UseAfterMove ,
+   const LambdaExpr *Lambda, ClangTidyCheck *Check,
ASTContext *Context) {
   SourceLocation UseLoc = Use.DeclRef->getExprLoc();
   SourceLocation MoveLoc = MovingCall->getExprLoc();
+  const bool MoveArgIsConst = MoveArg->getType().isConstQualified();
 
-  Check->diag(UseLoc, "'%0' used after it was moved")
-  << MoveArg->getDecl()->getName();
+  Check->diag(UseLoc, "'%0' used after it was moved%select{|; move of a 'const' argument has no effect}1")
+  << MoveArg->getDecl()->getName()
+  << (MoveArgIsConst ? 1 : 0);
   Check->diag(MoveLoc, "move occurred here", DiagnosticIDs::Note);
+  if (MoveArgIsConst) {
+if (Lambda) {
+  for (auto Capture = Lambda->capture_begin(), End = Lambda->capture_end();
+   Capture != End; ++Capture) {
+if (MoveArg->getDecl() == Capture->getCapturedVar()) {
+  const bool IsExplicitCapture = Capture->isExplicit();
+  Check->diag(
+  IsExplicitCapture ? Capture->getLocation()
+: Lambda->getCaptureDefaultLoc(),
+  "variable %0 %select{|implicitly}1 captured as const here",
+  DiagnosticIDs::Note)
+  << Capture->getCapturedVar() << (IsExplicitCapture ? 0 : 1);
+  break;
+}
+  }
+} else {
+  Check->diag(MoveArg->getDecl()->getLocation(),
+  "variable '%0' declared as const 

[PATCH] D74692: [clang-tidy][bugprone-use-after-move] Warn on std::move for consts

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp:399-400
+Check->diag(MoveArgLoc,
+"std::move of the const expression has no effect; "
+"remove std::move() or make the variable non-const",
+DiagnosticIDs::Note);

I don't think this diagnostic makes sense on the location of the declaration -- 
the diagnostic is talking about expressions but the code the user is looking at 
is a declaration. I think it may make more sense to change the diagnostic at 
the use location to read `'%0' used after it was moved; move of a 'const' 
argument has no effect` and then this note can read `variable '%0' declared 
const here` (so long as you get the correct declaration location -- the lambda 
example in the test below would be incorrect, for instance).



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp:329
   // CHECK-NOTES: [[@LINE-3]]:7: note: move occurred here
+  // CHECK-NOTES: [[@LINE-6]]:7: note: std::move of the const expression 
{{.*}}
 };

zinovy.nis wrote:
> Quuxplusone wrote:
> > It continues to seem silly to me that you give an extra note here saying 
> > that line 325 doesn't do anything, when of course line 336 doesn't do 
> > anything either (and you don't give any extra note there).
> > 
> > This clang-tidy warning isn't supposed to be about what //physically 
> > happens// in the machine code during any particular compilation run; it's 
> > supposed to be about helping the user avoid //semantic// bugs by cleaning 
> > up their codebase's //logical// behavior. The rule is "don't use things 
> > after moving from them," period.
> > 
> > Analogously, if there were a clang-tidy warning to say "always indent four 
> > spaces after an `if`," and you proposed to add a note to some cases that 
> > said "...but here a three-space indent is OK, because C++ is 
> > whitespace-insensitive" — I'd also find //that// note to be mildly 
> > objectionable. We want to train people to do the right thing, not to do the 
> > right thing "except in this case because hey, it doesn't matter to the 
> > machine."
> Thanks for the feedback. I got your point. But my note (may be expressed with 
> wrong words) is about that there're 2 ways to fix the issue: either get rid 
> of 'std::move' or somehow remove 'const'. That was the main purpose of my 
> commit.
In this particular instance, I find the new note to be perplexing -- the 
constant expression being referenced is the original declaration of `a` which 
is not `const` (the captured `a` within the lambda is what's `const`).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74692/new/

https://reviews.llvm.org/D74692



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74692: [clang-tidy][bugprone-use-after-move] Warn on std::move for consts

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis marked an inline comment as done.
zinovy.nis added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp:329
   // CHECK-NOTES: [[@LINE-3]]:7: note: move occurred here
+  // CHECK-NOTES: [[@LINE-6]]:7: note: std::move of the const expression 
{{.*}}
 };

Quuxplusone wrote:
> It continues to seem silly to me that you give an extra note here saying that 
> line 325 doesn't do anything, when of course line 336 doesn't do anything 
> either (and you don't give any extra note there).
> 
> This clang-tidy warning isn't supposed to be about what //physically 
> happens// in the machine code during any particular compilation run; it's 
> supposed to be about helping the user avoid //semantic// bugs by cleaning up 
> their codebase's //logical// behavior. The rule is "don't use things after 
> moving from them," period.
> 
> Analogously, if there were a clang-tidy warning to say "always indent four 
> spaces after an `if`," and you proposed to add a note to some cases that said 
> "...but here a three-space indent is OK, because C++ is 
> whitespace-insensitive" — I'd also find //that// note to be mildly 
> objectionable. We want to train people to do the right thing, not to do the 
> right thing "except in this case because hey, it doesn't matter to the 
> machine."
Thanks for the feedback. I got your point. But my note (may be expressed with 
wrong words) is about that there're 2 ways to fix the issue: either get rid of 
'std::move' or somehow remove 'const'. That was the main purpose of my commit.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74692/new/

https://reviews.llvm.org/D74692



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74692: [clang-tidy][bugprone-use-after-move] Warn on std::move for consts

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp:329
   // CHECK-NOTES: [[@LINE-3]]:7: note: move occurred here
+  // CHECK-NOTES: [[@LINE-6]]:7: note: std::move of the const expression 
{{.*}}
 };

It continues to seem silly to me that you give an extra note here saying that 
line 325 doesn't do anything, when of course line 336 doesn't do anything 
either (and you don't give any extra note there).

This clang-tidy warning isn't supposed to be about what //physically happens// 
in the machine code during any particular compilation run; it's supposed to be 
about helping the user avoid //semantic// bugs by cleaning up their codebase's 
//logical// behavior. The rule is "don't use things after moving from them," 
period.

Analogously, if there were a clang-tidy warning to say "always indent four 
spaces after an `if`," and you proposed to add a note to some cases that said 
"...but here a three-space indent is OK, because C++ is whitespace-insensitive" 
— I'd also find //that// note to be mildly objectionable. We want to train 
people to do the right thing, not to do the right thing "except in this case 
because hey, it doesn't matter to the machine."


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74692/new/

https://reviews.llvm.org/D74692



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits