The convention to pass a parameter when ownership transfer is optional

[jww...@mozilla.com]

https://hg.mozilla.org/mozilla-central/file/3e04659fdf6aef792f7cf9840189c6c38d08d1e8/mfbt/UniquePtr.h#l182

"To unconditionally transfer ownership of a UniquePtr into a method, use a |UniquePtr| argument. To conditionally transfer ownership of a resource into a method, should the method want it, use a |UniquePtr&&| argument."

Does that also apply to already_AddRefed<>&& or we stick to https://bugzilla.mozilla.org/show_bug.cgi?id=1247972#c19?

Btw, we have some code like https://hg.mozilla.org/mozilla-central/file/3e04659fdf6aef792f7cf9840189c6c38d08d1e8/dom/base/WebSocket.cpp#l2790 where it really should be just already_AddRefed<> since the ownership is always transferred.

[Kyle Huey]

Conceptually already_AddRefed<T> is a move reference. And it's destructor
asserts (in debug builds) that the reference was indeed moved, so it cannot
be used as a conditional transfer under any circumstances.

- Kyle

> _______________________________________________
> dev-platform mailing list
> dev-pl...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>

[Gerald Squelart]

One concern I have with foo(T&&) being used to mark optional transfer, is that on the caller side there will be a foo(Move(t)) followed by code that may *still* uses 't'.
I can't remember exactly where I read/heard that (Herb Sutter I think?), but it was advised that moved-from objects should only be destroyed, or completely overwritten through an assignment; and this philosophy was used to design STL containers move-related APIs. -- Or am I imagining things?


Specific classes (like UniquePtr) might in fact be designed for this particular idiom, and I think it's fine when fully documented and tested.

But it'd be great to come to agreement and document it in coding guidelines, whether we're going all guns blazing for T&&/Move-and-keep-using, or non-recommended-but-allowed-if-you-really-really-know-what-you're-doing, or something else entirely...

gerald

[jww...@mozilla.com]

I am tempted to assume |t| will be an empty shell after foo(Move(t)) if I don't see the prototype foo(T&&).

For |bar(already_AddRefed<T>&&)|, it is also possible for the callsite to say |bar(r.forget())| without forcing the caller to handle conditional ownership transfer which won't be caught until runtime.

[Xidorn Quan]

I used already_AddRefed<>&& (and even UniquePtr<>&&) for parameters for
unconditional transfer for quite a while. But I'm recently convinced that
we should use already_AddRefed<> and UniquePtr<> in almost all cases,
because compilers actually have more chance to optimize code with them
rather than &&.

I believe conditional move is rare, and could be confusing in most cases,
and you may actually want to use T& rather than T&& for conditional move.
The only use of && is for move-constructor and functions we want people to
reconsider when they try to pass in lvalue.

- Xidorn

[Bobby Holley]

Just piling on here - I think that the conditional-move idiom is super
confusing and we should forbid it. If some specific class needs such an
idiom, they can build a special container that can be passed by
lvalue-reference.

On Thu, Mar 17, 2016 at 11:14 PM, Xidorn Quan <quanx...@gmail.com> wrote:

> On Fri, Mar 18, 2016 at 11:07 AM, <jww...@mozilla.com> wrote:
>
> >
> >

> I used already_AddRefed<>&& (and even UniquePtr<>&&) for parameters for
> unconditional transfer for quite a while. But I'm recently convinced that
> we should use already_AddRefed<> and UniquePtr<> in almost all cases,
> because compilers actually have more chance to optimize code with them
> rather than &&.
>
> I believe conditional move is rare, and could be confusing in most cases,
> and you may actually want to use T& rather than T&& for conditional move.
> The only use of && is for move-constructor and functions we want people to
> reconsider when they try to pass in lvalue.
>
> - Xidorn