Closed Bug 1119086 Opened 5 years ago Closed 5 years ago

already_AddRefed should define copy/move assignment operators

Categories

(Core :: MFBT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: froydnj, Assigned: sciarp, Mentored)

Details

(Whiteboard: [lang=c++])

Attachments

(1 file, 1 obsolete file)

We've gotten away with it thus far, but we should really close this loophole.
Whiteboard: [lang=
Whiteboard: [lang= → [lang=c++]
Is someone working on this bug ? If not, I would like to give it a try.
I'd like to try it too.
Hi, I would like to work on this bug. I am really new to opensource and don't know where to start. This would be my first bug. Could someone please tell me what i should do to work on it ?
The copy constructor is disabled shall we disable the copy assignment as well and only use move semantics?
Flags: needinfo?(nfroyd)
(In reply to Donato Sciarra from comment #5)
> The copy constructor is disabled shall we disable the copy assignment as
> well and only use move semantics?

Yes.
Flags: needinfo?(nfroyd)
Attached patch bug1119086.patch (obsolete) — Splinter Review
Attachment #8590870 - Flags: review?(nfroyd)
Comment on attachment 8590870 [details] [diff] [review]
bug1119086.patch

Review of attachment 8590870 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch!  A few comments below.

::: mfbt/AlreadyAddRefed.h
@@ +72,4 @@
>  
>    already_AddRefed(already_AddRefed<T>&& aOther) : mRawPtr(aOther.take()) {}
>  
> +  already_AddRefed<T>& operator=(const already_AddRefed<T>&& aOther)

Move assignment operators are typically declared:

... operator=(T&& aOther)

since presumably you need to modify aOther's internal state to move that state into |this|.  (FWIW, C++ permits you to define |operator=(const T&& aOther)|, says it is recognized as a move assignment operator (!), *and* permits you to define |operator=(const T&& aOther)| alongside |operator=(T&& aOther)|.  Why it does this I do not know.)

@@ +73,5 @@
>    already_AddRefed(already_AddRefed<T>&& aOther) : mRawPtr(aOther.take()) {}
>  
> +  already_AddRefed<T>& operator=(const already_AddRefed<T>&& aOther)
> +  {
> +    if(this != &aOther)

We tend to not guard against self-assignment in our operator= methods.  And I think even in the unusual event of self-assignment, this method would do the right thing.

Please remove this if-check.

@@ +75,5 @@
> +  already_AddRefed<T>& operator=(const already_AddRefed<T>&& aOther)
> +  {
> +    if(this != &aOther)
> +    {
> +      mRawPtr = aOther.take();

I am a little surprised that this would compile, given that take() is a non-const method, and |aOther| is |const|.  Did it compile?
Attachment #8590870 - Flags: review?(nfroyd) → feedback+
Assignee: nobody → sciarp
Attached patch bug1119086.patchSplinter Review
Yes it did compile. I have anyway modified the patch to follow your suggestions.
Attachment #8590870 - Attachment is obsolete: true
Attachment #8591071 - Flags: review?(nfroyd)
Comment on attachment 8591071 [details] [diff] [review]
bug1119086.patch

Review of attachment 8591071 [details] [diff] [review]:
-----------------------------------------------------------------

Huh, I would not have expected the previous patch to compile.  Maybe const r-value references aren't really const?

Thanks for updating the patch.  This version looks good.
Attachment #8591071 - Flags: review?(nfroyd) → review+
I don't have yet access to try-server, before marking it as "check-in needed" could you please let it run on the try-server? Thanks!
Flags: needinfo?(nfroyd)
(In reply to Donato Sciarra from comment #11)
> I don't have yet access to try-server, before marking it as "check-in
> needed" could you please let it run on the try-server? Thanks!

Done: https://treeherder.mozilla.org/#/jobs?repo=try&revision=35d503ccf196
Flags: needinfo?(nfroyd)
Sigh, I botched the bug number on the try push.  Anyway, try build looks green (I think the B2G build failures are some sort of bad interaction with infra), requesting checkin-needed.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/72c7745a331a
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.