1143008 - Replace nsAutoPtr usages in StyleAnimationValue.cpp with UniquePtr

Status: NEW

(Core :: CSS Parsing and Computation, defect)

Description

[Nathan Froyd [:froydnj]]

I should preface this by saying that I'd like to essentially s/nsAutoPtr/UniquePtr/g, not just in StyleAnimationValue.cpp; consider this the first step towards doing so, with some stylistic questions on how that should be done.

The prevailing patterns in StyleAnimationValue.cpp looks like:

  nsAutoPtr<T> x(...);
  // do various things with x
  // do something with the result of x.forget(), a T*

e.g. http://mxr.mozilla.org/mozilla-central/source/layout/style/StyleAnimationValue.cpp#2101

or:

  nsAutoPtr<T> x;
  T** xTail = getterTransfers(x);
  // do various things appending to xTail
  // do something with the result of x.forget(), a T*

e.g. http://mxr.mozilla.org/mozilla-central/source/layout/style/StyleAnimationValue.cpp#3215

It's straightforward enough to use UniquePtr<T> instead of nsAutoPtr<T>; my question is really how appropriate spreading the UniquePtr<T> usage around is.  There's already conventions like |SetAndAdopt*|:

http://mxr.mozilla.org/mozilla-central/source/layout/style/StyleAnimationValue.h#350

which seem like they'd be slightly better off taking UniquePtr<T>&& to enforce the adoption in the type system, not just in the name.  (I'm assuming we can pass UniquePtr<T>&& for the same cost as raw pointers; that may not be absolutely true...)

ni?'ing dholbert for his thoughts.

Comment 1

[Daniel Holbert [:dholbert]]

I'm not intricately familiar with UniquePtr, but my general impression is that it's preferable to nsAutoPtr (particularly due to its reduced reliance on intermediate raw pointers, e.g. in foo.forget()). So, I'm OK at a high level with switching to it.

(Note to self, since I'd forgotten: For UniquePtr, ".forget()" is called ".release()".)

> my question is really how appropriate spreading the UniquePtr<T> usage around is

Probably good to have dbaron in the loop here, as style-system owner, to make sure he's OK with this. CC'ing him. Also CC'ing UniquePtr author Waldo, since he's more likely to know conversion-to-UniquePtr strategies than I am.

> There's already conventions like |SetAndAdopt*|:
> [...]which seem like they'd be slightly better off taking UniquePtr<T>&&

That seems maybe-debatable.  At least, the UniquePtr documentation says:
 * [...]  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.
http://mxr.mozilla.org/mozilla-central/source/mfbt/UniquePtr.h#148

Since we're wanting to *unconditionally* transfer ownership (per "Adopt" in the name), seems like the documentation says these methods should just take UniquePtr<T> -- no &&'s. (Though, this will introduce 1 extra assignment, to construct the UniquePtr arg, which raw pointers and UniquePtr<T>&& would not involve. Not sure if that's a cost worth worrying about. It does buy us a bit of extra leak-proofing, so maybe it's worth it.)

Also, note that SetAndAdopt* use a tagged union to store the adopted pointer -- I'm pretty sure we can't (or don't want to) use UniquePtr inside of that union. (We won't get the destructor-will-clean-us-up-for-free guarantees at least, since it's just a union.)  So we can't fully do away with raw pointers here. And maybe that means UniquePtr is actually no better than nsAutoPtr? (other than from a switching-to-a-single-consistent-style standpoint)