1309794 - Make RefCnt not copyable

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Xidorn Quan [:xidorn] UTC+11]

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 1

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1309794 - Make RefCnt types non-copyable.

Review commit: https://reviewboard.mozilla.org/r/85452/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/85452/

Comment 2

[Nathan Froyd [:froydnj]]

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?

Comment 3

[Nathan Froyd [:froydnj]]

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 4

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8800532 [details]
Bug 1309794 - Make RefCnt types non-copyable.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85452/diff/1-2/

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/971b4fc7d1f4
Make RefCnt types non-copyable. r=froydnj

Comment 6

[James Cheng[:JamesCheng]]

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?

Comment 7

[Xidorn Quan [:xidorn] UTC+11]

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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/971b4fc7d1f4

Comment 9

[Nathan Froyd [:froydnj]]

(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