Once we can guarantee we can't shoot ourselves in the foot via: RefPtr<T> Foo(); T* p = Foo(); we can think about removing already_AddRefed in favor of simply returning RefPtr<T> or similar everywhere. I think that was the biggest objection to transitioning to a more C++11-style of simply returning smart pointers rather than passing this notational temporary object. I think there's some value is seeing already_AddRefed and thinking of that as ownership transfer. Having already_AddRefed function parameters also encourages people to be explicit about reference counting, whether via RefPtr::forget() or MakeAndAddRef or similar. But these are probably minor concerns, and Jeff has already argued with me that these are not worth it for the hassle of understanding this extra class.

Michael has talked about adding a static analysis that will catch situations where is a parameter is copied into a single location instead of being moved. That should ensure that any places where we are using already_AddRefed parameters today we properly move RefPtrs

Given that AddRef/Release are often slow operations, hiding them is rather bad practice. Can we _guarantee_ that extra AddRef/Release are optimized out when returning RefPtr<T> (it is unclear to me how that could happen in case of virtual AddRef/Release and such)?

(In reply to Olli Pettay [:smaug] (high review load, please consider other reviewers) from comment #2) > Given that AddRef/Release are often slow operations, hiding them is rather > bad practice. > Can we _guarantee_ that extra AddRef/Release are optimized out when > returning RefPtr<T> > (it is unclear to me how that could happen in case of virtual AddRef/Release > and such)? If we write: return Move(refptr); then we will theoretically (assuming that no optimizations are performed), generate a call to the move constructor, which doesn't AddRef/Release. The value will be theoretically constructed in a temporary which is then returned, and moved again into the value at the return site. Theoretically, if the `forget` method was defined as RefPtr<T> forget() { return Move(this); } we can even write the code in the same way it's currently written, as the return value of this function would be an rvalue and thus moved. I believe however that RVO will occur in all of our compilers, meaning that this explicit ownership transfer shouldn't be necessary. Botond might know more.

What's the story for parameters? Is it possible to require callsites to move the pointer by default? (I guess you can do that via using RefPtr<T>&& for type... which removes an optimization chance)

(In reply to Michael Layzell [:mystor] [: from comment #3) > If we write: > > return Move(refptr); > We'll we'd need to have some static analysis to force to return only this way, and not ever just return refptr.

(In reply to Michael Layzell [:mystor] from comment #3) > If we write: > > return Move(refptr); > > then we will theoretically (assuming that no optimizations are performed), > generate a call to the move constructor, which doesn't AddRef/Release. The > value will be theoretically constructed in a temporary which is then > returned, and moved again into the value at the return site. This is not necessary. When returning an object, the language already specifies that the object is *moved* (rather than copied) into the temporary you speak of (and then moved again into the value at the return site), without you having to write Move(). This is not an optimization, it's guaranteed. In fact, you can return an object (without calling Move()) that doesn't even have a copy constructor, just a move constructor. (One of both of the moves can then be elided. That *is* an optimization, until C++17, which guarantees the elision of the second move (and certain other elisions in other contexts).)

(In reply to Botond Ballo [:botond] (C++ standards meeting Jun 20-25) from comment #6) > (In reply to Michael Layzell [:mystor] from comment #3) > > If we write: > > > > return Move(refptr); > > > > then we will theoretically (assuming that no optimizations are performed), > > generate a call to the move constructor, which doesn't AddRef/Release. The > > value will be theoretically constructed in a temporary which is then > > returned, and moved again into the value at the return site. > > This is not necessary. When returning an object, the language already > specifies that the object is *moved* (rather than copied) into the temporary > you speak of (and then moved again into the value at the return site), > without you having to write Move(). This is not an optimization, it's > guaranteed. In fact, you can return an object (without calling Move()) that > doesn't even have a copy constructor, just a move constructor. > > (One of both of the moves can then be elided. That *is* an optimization, > until C++17, which guarantees the elision of the second move (and certain > other elisions in other contexts).) If that is guaranteed, then I don't think that we would need any such static analysis, as the program will already do the right thing. The only problem now is the strange callsites which store already_AddRefed in a struct or pass it by value as an argument I would think.

(In reply to Botond Ballo [:botond] (C++ standards meeting Jun 20-25) from comment #6) > This is not necessary. When returning an object, the language already Even true when returning a named object (lvalue)?

(In reply to JW Wang [:jwwang] from comment #8) > (In reply to Botond Ballo [:botond] (C++ standards meeting Jun 20-25) from > comment #6) > > This is not necessary. When returning an object, the language already > Even true when returning a named object (lvalue)? Yes. I believe the relevant standardese is [class.copy] p32: "... when the expression in a return statement is a (possibly parenthesized) id-expression that names an object with automation storage duration declared in the body or parameter-declaration-cause of the innermost enclosing function or lambda-expression, overload resolution to select the constructor for the copy is first performed as if the object were designated by an rvalue." So, if the type has a move constructor (as RefPtr does), the move constructor will be called rather than the copy constructor. The only thing that's not guaranteed, is that the call to the move constructor itself will be elided (optimized away).

(The idea behind this rule is: if the lvalue names an object that's about to be destroyed anyways (because it's local to the function), we might as well move from it.)

Can we probably just make RefPtr move-only, and add an method, e.g. Clone(), to it in case people do want to add ref?

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #11) > Can we probably just make RefPtr move-only, and add an method, e.g. Clone(), > to it in case people do want to add ref? That sounds nice. It will make using RefPtr consistent with UniquePtr. It will be like rust's Rc<T>. Now only if the rewrite won't be too massive. Dxr shows there are 21,788 results matching RefPtr

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #11) > Can we probably just make RefPtr move-only, and add an method, e.g. Clone(), > to it in case people do want to add ref? So you're proposing that RefPtrs would be always created using MakeRefPtr or such?

(In reply to Olli Pettay [:smaug] from comment #13) > (In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #11) > > Can we probably just make RefPtr move-only, and add an method, e.g. Clone(), > > to it in case people do want to add ref? > So you're proposing that RefPtrs would be always created using MakeRefPtr or > such? No. Why do you think I mean that? I was proposing marking the copy constructor of RefPtr "= delete" so that people cannot copy it by accident, and consequently we can pass RefPtr by value everywhere without worrying about people forgetting to add Move() around lvalues. But that is probably a too massive change to do.

So how would the setup work then? RefPtr<Foo> foo = SomeFoo; Method(foo); and passing to Method would steal the pointer from foo? I guess I'm misunderstanding the proposal.

(In reply to Olli Pettay [:smaug] from comment #15) > So how would the setup work then? > > RefPtr<Foo> foo = SomeFoo; > Method(foo); > > and passing to Method would steal the pointer from foo? If Method takes a RefPtr<Foo>, compiling would fail, because there is no copy ctor. You would need to either Move(foo) or foo.Clone() explicitly. If Method takes Foo*, we can still have RefPtr<Foo> implicitly converted to Foo*, and nothing is changed then. It would just require you to make clear decision when you are assigning a RefPtr to another RefPtr (which currently uses copy ctor implicitly). For comment 0, now we can add > operator T*() && = delete; to RefPtr to forbid assigning an rvalue RefPtr<Foo> to Foo* without affecting most valid conversions.

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #16) > For comment 0, now we can add > > operator T*() && = delete; > to RefPtr to forbid assigning an rvalue RefPtr<Foo> to Foo* without > affecting most valid conversions. We have http://searchfox.org/mozilla-central/source/mfbt/RefPtr.h#294.

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #16) > If Method takes a RefPtr<Foo>, compiling would fail, because there is no > copy ctor. You would need to either Move(foo) or foo.Clone() explicitly. If > Method takes Foo*, we can still have RefPtr<Foo> implicitly converted to > Foo*, and nothing is changed then. I see. If we go with this route, we would by default convert all the param passing to use .Clone() since Move changes lifetime management radically and all the changes would need to be reviewed very carefully in order to make sure it is fine to possibly delete the object earlier. But of course RefPtr as a param is rare, so it should be fine. > It would just require you to make clear decision when you are assigning a > RefPtr to another RefPtr (which currently uses copy ctor implicitly). Well, shouldn't operator= still work? So explicit copy ctor usage would just need to changed to assignment.