On Mon, Aug 15, 2016 at 3:55 PM, Aryeh Gregor <a...@aryeh.name> wrote:
> IMO, it makes sense to move ahead with this.

One more point I forgot to (re-?)mention: with proper move
constructors, it's not just already_AddRefed<T> return values that
should be changed to RefPtr<T>, but also T* return values, in the case
where the returning function already had a RefPtr<T> lying around.
This will *reduce* addref/release in common cases, like:

  T*
  Foo()
  {
    RefPtr<T> t = DoStuff();
    return t;
  }

  RefPtr<T> t = Foo();

Currently this does a release when Foo() returns, and a new addref
when assigning to the new variable.  If Foo() instead returned
RefPtr<T> with proper move constructors, these would be removed.  You
also could not assign the return value of Foo() to a raw pointer
(without .get()), which improves security against use-after-free
without any extra addref/release.  (The release is just handled by the
caller instead of callee.)

It could inadvertently introduce extra addref/release if a function
that doesn't hold a reference on some code paths has its return type
changed to RefPtr, like:

  T* Bar();

  T*
  Foo()
  {
    return Bar();
  }

  Foo()->Method();

If Foo()'s return type was changed to RefPtr<T>, this would silently
add an addref/release.  I don't know how to prevent this.  Would
making the relevant constructor explicit do it?  If so, would it cause
issues in other places?

Also -- now I'm remembering more of the issues here -- this would also
require .get() to pass a function's return value directly as a
function parameter, unless bug 1194215 is fixed (see also bug
1194195).  It's not *so* common to do this, but it's common enough
that we don't want to require .get(), I think.

So this isn't such a no-brainer, but I think it is where we ideally want to be.
_______________________________________________
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Reply via email to