std::move(x) should be preferred over x.forget() in construction/assignment
Categories
(Developer Infrastructure :: Source Code Analysis, task)
Tracking
(firefox74 fixed)
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
Comment 1•5 years ago
|
||
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
.
Updated•5 years ago
|
Assignee | ||
Comment 2•5 years ago
|
||
Assignee | ||
Comment 3•5 years ago
|
||
Assignee | ||
Comment 4•5 years ago
|
||
Depends on D60979
Comment 6•5 years ago
|
||
bugherder |
Assignee | ||
Updated•5 years ago
|
Comment 8•5 years ago
|
||
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/logviewer.html#/jobs?job_id=288561424&repo=autoland&lineNumber=14785
Comment 10•5 years ago
|
||
This caused multiple failures, not just build bustages:
Updated•5 years ago
|
Comment 11•5 years ago
|
||
Comment 12•5 years ago
|
||
bugherder |
Assignee | ||
Comment 13•5 years ago
|
||
Clear need-info, the fixed patch has landed in the meantime.
Updated•5 years ago
|
Comment 14•4 years ago
|
||
The leave-open keyword is there and there is no activity for 6 months.
:sg, maybe it's time to close this bug?
Assignee | ||
Comment 15•4 years ago
|
||
Remaining patch D60955 moved over to new Bug 1662652.
Assignee | ||
Updated•4 years ago
|
Comment 16•4 years ago
|
||
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.
Updated•4 years ago
|
Updated•2 years ago
|
Description
•