1611415 - std::move(x) should be preferred over x.forget() in construction/assignment

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, task)

Description

[Simon Giesecke [:sg] [he/him]]

In particular with smart pointer class templates such as RefPtr and nsCOMPtr, custom ways of moving the content are used, which go back to pre-move semantics times. These included uses of already_AddRefed<T> forget() and swap methods. The former cases can easily be changed to use standard std::move, e.g.

  RefPtr<X> x = std::move(refPtr); // instead of RefPtr<X> x = refPtr.forget();
  x = std::move(refPtr2); // instead of x = refPtr2.forget();

These cases can be easily detected and transformed by a clang-based checker.

The benefits of this include

• improving readability by reducing the mixture of now-widespread std::move and custom non-standard moves
• enabling other static analyses, most importantly clang-tidy's bugprone-use-after-move

Comment 1

[Andi [:andi]]

Thanks for reporting this, I agree that we should move away from the old semantic that pre-dates cpp11, as a matter of fact we've already included in the analysis bugprone-use-after-move and it would help us greatly if we would change the forget semantic with std::move.

Comment 2

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1611415 - New non-standard move checker. r=andi

Comment 3

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1611415 - Make nsCOMPtr, StaticRefPtr and OwningNonNull constructible/assignable from RefPtr<U>&& without needing to go through already_AddRefed. r=andi,sylvestre

Comment 4

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1611415 - Prefer using std::move over forget. r=andi,sylvestre

Depends on D60979

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/81031230ca7d
Make nsCOMPtr, StaticRefPtr and OwningNonNull constructible/assignable from RefPtr<U>&& without needing to go through already_AddRefed. r=andi

Comment 6

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/81031230ca7d

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0c982bc69cb3
Applied FixItHints from mozilla-non-std-move. r=froydnj

Comment 8

[Stefan Hindli [:stefan_hindli]]

Backed out changeset 0c982bc69cb3 (Bug 1611415) for causing build bustages in /builds/worker/workspace/build/src/obj-firefox/dist/include/nsCOMPtr CLOSED TREE

https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&selectedJob=288561424

https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=288561424&repo=autoland&lineNumber=14785

Comment 9

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/40224b948004
Backed out changeset 0c982bc69cb3 for causing build bustages in /builds/worker/workspace/build/src/obj-firefox/dist/include/nsCOMPtr CLOSED TREE

Comment 10

[Andreea Pavel [:apavel]]

This caused multiple failures, not just build bustages:

https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&tochange=40224b9480044dc1937981702026d84a05794353&fromchange=0c982bc69cb321db4e4fd188c641505c26afab5b&failure_classification_id=2&selectedJob=288569893

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/49adcf9a70ab
Prefer using std::move over forget. r=froydnj

Comment 12

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/49adcf9a70ab

Comment 13

[Simon Giesecke [:sg] [he/him]]

Clear need-info, the fixed patch has landed in the meantime.

Comment 14

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

The leave-open keyword is there and there is no activity for 6 months.
:sg, maybe it's time to close this bug?

Comment 15

[Simon Giesecke [:sg] [he/him]]

Remaining patch D60955 moved over to new Bug 1662652.

Comment 16

[Phabricator Automation]

Comment on attachment 9122868 [details]
Bug 1611415 - New non-standard move checker. r=andi

Revision D60955 was moved to bug 1662652. Setting attachment 9122868 [details] to obsolete.