1558359 - [Automated review] Unhelpful suggestion to remove std::move of trivially-copyable type

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, task)

Description

[Jim Blandy :jimb]

Phabricator URL: https://phabricator.services.mozilla.com/D32272#999353

I don't think this comment is helpful. The use in question is definitely a move; nobody needs to use the original value. Using std::move for a trivially copyable type shouldn't do any harm; the compiler should optimize it away. If the type changes to be no longer trivially-copyable, there will be no compiler error or warning letting us know that we should add back the std::move.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug is a task, but please change it back in case of error.

Comment 2

[Andi [:andi]]

:jimb I see your point, but in the end in the current state of the code we don't actually need std::move. I see your concern here, but on the other hand do we really need to have this stuff in code in order to make it more complex and in the end they don't bring anything to the table? The role of this checker is more informative.

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

This has come up for my team and I did a little digging. It appears that https://clang.llvm.org/extra/clang-tidy/checks/performance-move-const-arg.html might have grown out of "hicpp-move-const-arg" (https://clang.llvm.org/extra/clang-tidy/checks/hicpp-move-const-arg.html that redirects after a few seconds) which cites 17.3.1 of https://www.perforce.com/resources/qac/high-integrity-cpp-coding-standard/standard-library which is "17.3.1 Do not use std::move on objects declared with const or const & type" which does not involve trivially copyable. That rationale makes more sense than the trivially copyable check. And indeed, there is a "CheckTriviallyCopyableMove" option that we can disable.

Re: informative checks, the current UX of the reviewbot as it integrates with phabricator and the default phabricator setting of "Diff Preferences / Show Older Inlines" to "Enabled" (or at least it was for me) makes the reviewbot's presence on reviews frequently feel antagonistic. Especially in cases like this where the error is reported as high confidence but it's of "trivial" priority. For posterity, here's the current text from one instance (which is presented visually the same as comments from human reviewers or low confidence reviewbot comments, etc):

Warning: Std::move of the variable 'aGivenProto' of the trivially-copyable type 'JS::Handle<JSObject *>' has no effect; remove std::move() [clang-tidy: performance-move-const-arg]
Checker reliability is high, meaning that the false positive ratio is low.

That said, I do want to emphasize that I think static analysis is important. But the failure mode here is people silently ignoring the reviewbot because of perceived spamminess and not providing feedback on it because filing bugs that start out with "I've starting ignoring the static analysis..." seems like a good way to get a bad performance review.

Given that Ehsan is the owner of the relevant module (https://wiki.mozilla.org/Modules/All#C.2B.2B.2FRust_usage.2C_tools.2C_and_style), Ehsan do you have thoughts on disabling the CheckTriviallyCopyableMove option?

Comment 4

[(no longer active)]

(In reply to Andrew Sutherland [:asuth] (he/him) from comment #3)

This has come up for my team and I did a little digging. It appears that https://clang.llvm.org/extra/clang-tidy/checks/performance-move-const-arg.html might have grown out of "hicpp-move-const-arg" (https://clang.llvm.org/extra/clang-tidy/checks/hicpp-move-const-arg.html that redirects after a few seconds) which cites 17.3.1 of https://www.perforce.com/resources/qac/high-integrity-cpp-coding-standard/standard-library which is "17.3.1 Do not use std::move on objects declared with const or const & type" which does not involve trivially copyable. That rationale makes more sense than the trivially copyable check. And indeed, there is a "CheckTriviallyCopyableMove" option that we can disable.

I may be wrong but I believe https://reviews.llvm.org/D39863 is the specific change relevant to the diagnostic produced here. The discussion on the review is informative. Long story short: this warning is intended to inform the author that they've written useless code and the move will in fact be a copy. The usefulness of that message probably depends on the author and the context... But clearly as you can see from that review, it wasn't the intention of the author of this warning for it to be enabled by default.

Re: informative checks, the current UX of the reviewbot as it integrates with phabricator and the default phabricator setting of "Diff Preferences / Show Older Inlines" to "Enabled" (or at least it was for me) makes the reviewbot's presence on reviews frequently feel antagonistic. Especially in cases like this where the error is reported as high confidence but it's of "trivial" priority. For posterity, here's the current text from one instance (which is presented visually the same as comments from human reviewers or low confidence reviewbot comments, etc):

Warning: Std::move of the variable 'aGivenProto' of the trivially-copyable type 'JS::Handle<JSObject *>' has no effect; remove std::move() [clang-tidy: performance-move-const-arg]
Checker reliability is high, meaning that the false positive ratio is low.

Ah, this is not good. I can see how "checker reliability" may be confused with "the seriousness of the check". :-(

That said, I do want to emphasize that I think static analysis is important. But the failure mode here is people silently ignoring the reviewbot because of perceived spamminess and not providing feedback on it because filing bugs that start out with "I've starting ignoring the static analysis..." seems like a good way to get a bad performance review.

Given that Ehsan is the owner of the relevant module (https://wiki.mozilla.org/Modules/All#C.2B.2B.2FRust_usage.2C_tools.2C_and_style), Ehsan do you have thoughts on disabling the CheckTriviallyCopyableMove option?

Yes, per above I definitely think CheckTriviallyCopyableMove should be disabled by default on Phabricator.

Andi, do you happen to have cycles for this? Thanks!

Comment 5

[Andi [:andi]]

Try is very happy wit the new modification: https://treeherder.mozilla.org/#/jobs?repo=try&revision=22482e1c2b44eccc886bd0e6758e7fccb3b0fb12&selectedJob=271495690

Comment 6

[Andi [:andi]]

Attachment: Bug 1558359 - clang-tidy - For `performance-move-const-arg` disable detection of trivially copyable types that do not have a move constructor. r=sylvestre

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4ff3f106ce2f
clang-tidy - For `performance-move-const-arg` disable detection of trivially copyable types that do not have a move constructor. r=sylvestre

Comment 8

[Jim Blandy :jimb]

\o/ Thanks!

Comment 9

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/4ff3f106ce2f