Closed Bug 1611415 Opened 5 years ago Closed 4 years ago

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

Categories

(Developer Infrastructure :: Source Code Analysis, task)

task
Not set
normal

Tracking

(firefox74 fixed)

RESOLVED FIXED
mozilla74
Tracking Status
firefox74 --- fixed

People

(Reporter: sg, Assigned: sg)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

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

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.

Component: Lint and Formatting → Source Code Analysis
Attached file Bug 1611415 - New non-standard move checker. r=andi (obsolete) (deleted) —
Pushed by sgiesecke@mozilla.com: 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
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla74
Status: RESOLVED → REOPENED
Keywords: leave-open
Resolution: FIXED → ---
Pushed by sgiesecke@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0c982bc69cb3 Applied FixItHints from mozilla-non-std-move. r=froydnj
Backout by shindli@mozilla.com: 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
Attachment #9122909 - Attachment description: Bug 1611415 - Applied FixItHints from mozilla-non-std-move. r=andi,sylvestre → Bug 1611415 - Prefer using std::move over forget. r=andi,sylvestre
Pushed by sgiesecke@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/49adcf9a70ab Prefer using std::move over forget. r=froydnj

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

Flags: needinfo?(sgiesecke)

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

Flags: needinfo?(sgiesecke)
Blocks: 1662652

Remaining patch D60955 moved over to new Bug 1662652.

Flags: needinfo?(sgiesecke)
Status: REOPENED → RESOLVED
Closed: 5 years ago4 years ago
Resolution: --- → FIXED

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.

Attachment #9122868 - Attachment is obsolete: true
Product: Firefox Build System → Developer Infrastructure
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: