Make RefCnt not copyable

RESOLVED FIXED in Firefox 52

Status

()

Core
XPCOM
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: xidorn, Assigned: xidorn)

Tracking

Trunk
mozilla52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

It feels pretty broken if RefCnt is naively copied. Fortunately we don't have anything doing that currently. We want to avoid that from happening, I believe.
Comment hidden (mozreview-request)
Comment on attachment 8800532 [details]
Bug 1309794 - Make RefCnt types non-copyable.

https://reviewboard.mozilla.org/r/85452/#review84096

Makes sense to me.  Do we want to do this for the MFBT equivalent--which nobody should be using anyway--as well?
Attachment #8800532 - Flags: review?(nfroyd) → review+
And I think the advice is generally to delete operator=(const T&) when you delete the copy constructor, so we probably want to do that as well.
Comment hidden (mozreview-request)

Comment 5

a year ago
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/971b4fc7d1f4
Make RefCnt types non-copyable. r=froydnj

Comment 6

a year ago
mozreview-review
Comment on attachment 8800532 [details]
Bug 1309794 - Make RefCnt types non-copyable.

https://reviewboard.mozilla.org/r/85452/#review84378

::: xpcom/glue/nsISupportsImpl.h:297
(Diff revision 2)
>  public:
>    ThreadSafeAutoRefCnt() : mValue(0) {}
>    explicit ThreadSafeAutoRefCnt(nsrefcnt aValue) : mValue(aValue) {}
>  
> +  ThreadSafeAutoRefCnt(const ThreadSafeAutoRefCnt&) = delete;
> +  void operator=(const ThreadSafeAutoRefCnt&) = delete;

I think the prototype of the compiler default generated copy assignment is Class& operator=(const Class&) = default;

I think it is better to modify it instead of return void.

What do you think?
I don't think the return type here matters at all, given the functions are removed. The reason I use void is that the prototype of normal operator=() for nsCycleCollectingAutoRefCnt is too long to fit into one line, and I don't want to make it wrap nor exceed the line length limit.

Comment 8

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/971b4fc7d1f4
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
(In reply to James Cheng[:JamesCheng] from comment #6)
> ::: xpcom/glue/nsISupportsImpl.h:297
> (Diff revision 2)
> >  public:
> >    ThreadSafeAutoRefCnt() : mValue(0) {}
> >    explicit ThreadSafeAutoRefCnt(nsrefcnt aValue) : mValue(aValue) {}
> >  
> > +  ThreadSafeAutoRefCnt(const ThreadSafeAutoRefCnt&) = delete;
> > +  void operator=(const ThreadSafeAutoRefCnt&) = delete;
> 
> I think the prototype of the compiler default generated copy assignment is
> Class& operator=(const Class&) = default;
> 
> I think it is better to modify it instead of return void.

I think it would have been better to do this as well, but Xidorn is correct: as long as you declare a copy assignment operator (one of operator=(const T&), operator=(T&), or operator=(T); we'll ignore volatile qualifications of the parameter type, and note that the return type doesn't matter, only the parameter type), the generation of the implicit copy assignment operator is suppressed.  Welcome to C++! =D
You need to log in before you can comment on or make changes to this bug.