Page MenuHomePhabricator

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

Authored by zinovy.nis on Feb 16 2020, 7:25 AM.

Details

Summary

std::move for const values is NO-OP, so it makes sense to generate an additional note about this (similar to bugprone-use-after-move):

void simpleConst(const A a) {
  A other_a = std::move(a); // note: variable 'a' declared const here
  a.foo();                  // note: 'a' used after it was moved; move of a 'const' argument has no effect [bugprone-use-after-move]
}

Anyway, redundant std::move for consts can also be found with performance-move-const-arg check.

Diff Detail

Event Timeline

zinovy.nis created this revision.Feb 16 2020, 7:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 16 2020, 7:25 AM
zinovy.nis edited the summary of this revision. (Show Details)
zinovy.nis edited the summary of this revision. (Show Details)Feb 16 2020, 7:29 AM
Eugene.Zelenko set the repository for this revision to rG LLVM Github Monorepo.
Eugene.Zelenko removed a subscriber: Restricted Project.

I've added some individual comments above, but I'd question whether it's really necessary to make this change? Even if no move actually happens, the code is still wrong (it shouldn't be using std::move in the first place), and it's likely the author actually intended for a move to happen, so we should warn that that would cause a use-after-move.

clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
425

Is this in the right place? It looks as if it's applying to the std::move, not its argument?

clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp
137

Generally, I would avoid CHECK-NOTES-NOT if possible. Just leave off the comments entirely; the test will fail if there were any unexpected diagnostics, and the test is more resilient this way.

Even if no move actually happens, the code is still wrong

IMHO, it's not wrong, but it's inefficient. And will be reported by performance-move-const-arg.

(I sent this to the mailing list, but I guess it doesn't show up here unless I do it through Phab. Quoting myself—)

I see your point about how users who care should always be passing this check alongside "performance-move-const-arg"; but IMHO it still makes sense for clang-tidy to warn unconditionally about code of the form

x = std::move(y);
use(y);

regardless of the incidental type of y. Sure, it's technically 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 works in practice, such as std::list<int>... but regardless of its technical merits, the code is still logically semantically wrong, and I think that's what the clang-tidy check should be trying to diagnose.

(I sent this to the mailing list, but I guess it doesn't show up here unless I do it through Phab. Quoting myself—)

I see your point about how users who care should always be passing this check alongside "performance-move-const-arg"; but IMHO it still makes sense for clang-tidy to warn unconditionally about code of the form

x = std::move(y);
use(y);

regardless of the incidental type of y. Sure, it's technically 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 works in practice, such as std::list<int>... but regardless of its technical merits, the code is still logically semantically wrong, and I think that's what the clang-tidy check should be trying to diagnose.

Ok, maybe it worth just adding an additional text into the warning, like "Use of potentially moved value detected. But value is const so no actual move occurrs"?

Warning on use after move is still generated. But there's also an additional note

std::move of the const expression has no effect; remove std::move() or make the variable non-const

zinovy.nis edited the summary of this revision. (Show Details)Feb 25 2020, 9:49 PM
zinovy.nis retitled this revision from [clang-tidy] Make bugprone-use-after-move ignore std::move for const values to [clang-tidy][bugprone-use-after-mnove] Warn on std::move for consts.
Quuxplusone retitled this revision from [clang-tidy][bugprone-use-after-mnove] Warn on std::move for consts to [clang-tidy][bugprone-use-after-move] Warn on std::move for consts.Feb 26 2020, 6:44 AM
Quuxplusone added inline comments.Feb 26 2020, 6:53 AM
clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp
329

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."

zinovy.nis marked an inline comment as done.Feb 26 2020, 7:56 AM
zinovy.nis added inline comments.
clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp
329

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.

aaron.ballman added inline comments.Feb 27 2020, 6:56 AM
clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
419–420

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).

clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp
329

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).

  1. Special handling for captured variables in lambdas,
  2. messages texts were changed.
zinovy.nis marked an inline comment as done.Mar 3 2020, 11:40 AM
zinovy.nis marked an inline comment as done.

Typo fixed.

zinovy.nis marked 2 inline comments as done.Mar 3 2020, 12:04 PM
zinovy.nis edited the summary of this revision. (Show Details)

Any further comments?

aaron.ballman added inline comments.Mar 6 2020, 5:37 AM
clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
381

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.

389–390

Range-based for loop over captures()?

392

Same suggestions here as above.

clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp
342

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
}
zinovy.nis marked 4 inline comments as done.

Cosmetic and style fixes.

clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp
342

a in lambda is const, but it's not moved inside lambda, so my warning is not expected to be shown.

zinovy.nis marked an inline comment as done.Mar 6 2020, 11:44 PM
Quuxplusone added inline comments.Mar 7 2020, 7:26 AM
clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp
342

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.)

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.

mgehre removed a subscriber: mgehre.Mar 7 2020, 3:54 PM

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?

zinovy.nis marked 2 inline comments as done.Mar 8 2020, 3:15 AM
zinovy.nis added inline comments.
clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp
342

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

aaron.ballman accepted this revision.Mar 14 2020, 11:03 AM

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

clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
381

Top-level const is still here on the declaration.

391

Top-level const is still here as well.

This revision is now accepted and ready to land.Mar 14 2020, 11:03 AM

Removed top-level consts.

zinovy.nis marked 2 inline comments as done.Mar 14 2020, 12:06 PM

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.

zinovy.nis added a comment.EditedMar 15 2020, 3:10 AM

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 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.

zinovy.nis updated this revision to Diff 255128.Apr 5 2020, 2:14 AM

Rebase over the current master.

One more gentle ping)

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

zinovy.nis abandoned this revision.Apr 22 2020, 11:56 AM