Remove all uses of mfbt/Scoped.h, use mfbt/UniquePtr.h instead

REOPENED
Assigned to

Status

()

defect
REOPENED
5 years ago
3 months ago

People

(Reporter: Waldo, Assigned: Waldo, Mentored)

Tracking

(Blocks 1 bug, {leave-open})

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments)

Assignee

Description

5 years ago
UniquePtr is the long-term solution to what Scoped was made for, so Scoped uses should be removed.

Some of the uses of this are pretty easy to remove.  ScopedDeletePtr<T> can be changed almost without thought to UniquePtr<T>.  ScopedDeleteArray<T> can also be changed to UniquePtr<T[]>.

Other uses of Scoped may require more work to adjust.  Generally, if custom traits are specified, users will need to implement a deleter class that does the same things the scoped traits did.

I'm happy to mentor at least the ScopedDeletePtr/ScopedDeleteArray changes.  I may be able to mentor additional adjustments, but the more you get into customized Scoped traits, the better it probably is for the changes to be reviewed by owners of the relevant code.
Assignee

Updated

5 years ago
Blocks: 1037103
Assignee

Comment 1

5 years ago
There's very little ScopedDeleteArray around, might as well remove all of them so we can remove it from Scoped.h.  Cordon off the damage from this header, so to speak.
Attachment #8454251 - Flags: review?(jgilbert)
Assignee

Updated

5 years ago
Assignee: nobody → jwalden+bmo
Status: NEW → ASSIGNED
Assignee

Comment 2

5 years ago
Nothing too screwball here.  I would have propagated UniquePtr a little further into some of these methods, but those methods are in C files (boo!) so no dice, have to pass a .get() even tho logically ownership should probably transfer.
Attachment #8454252 - Flags: review?(ekr)
Assignee

Comment 3

5 years ago
The MappableExtractFile::Create bits are kind of nice -- we create the path, briefly use it, then seamlessly move it into an AutoUnlinkFile, then seamlessly move that further along into the ultimate owning structure.  No chance of errors or leaks at all along the way.  (As I recall we should do the same thing to MappableExtractFile as returned by that method, but there's some confusion there about whether MEF is refcounted or uniquely owned, so punting that to later.)
Attachment #8454253 - Flags: review?(mh+mozilla)
Assignee

Comment 4

5 years ago
This is just a grab-bag of places updated to UniquePtr<T[]>.  Nothing complicated, nothing warranting subject-matter scrutiny, or so it seems to me.  (Just a try run, forthcoming at some point.)

It was tempting to use |auto| for variable declarations assigned a MakeUnique<>(), but it may be too early to assume people will see that and successfully recognize it as being effectively just a new T(...).  For now, at least, go with repeating oneself very slightly.
Attachment #8454255 - Flags: review?(Pidgeot18)
Attachment #8454251 - Flags: review?(jgilbert) → review+
https://hg.mozilla.org/mozilla-central/rev/49d7327beb30
https://hg.mozilla.org/mozilla-central/rev/939d6ea39a64
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Comment on attachment 8454255 [details] [diff] [review]
Remove all use of ScopedDeleteArray from miscellaneous places

Review of attachment 8454255 [details] [diff] [review]:
-----------------------------------------------------------------

That MakeUnique sure is helpful! :-)
Attachment #8454255 - Flags: review?(Pidgeot18) → review+
Assignee

Updated

5 years ago
Status: RESOLVED → REOPENED
Keywords: leave-open
Resolution: FIXED → ---
Comment on attachment 8454253 [details] [diff] [review]
Remove all use of ScopedDeleteArray from mozglue/

Review of attachment 8454253 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozglue/linker/Mappable.cpp
@@ +90,4 @@
>      ERROR("Couldn't open %s to decompress library", path.get());
>      return nullptr;
>    }
> +  AutoUnlinkFile file(path.release());

Why not Move(path)?

@@ +396,4 @@
>    if (!mappable->buffer)
>      return nullptr;
>  
> +  mappable->chunkAvail = MakeUnique<unsigned char[]>(mappable->zStream.GetChunksNum());

I don't think this is going to call calloc under the hood. The memset should still be necessary.
Attachment #8454253 - Flags: review?(mh+mozilla) → review+
Assignee

Comment 8

5 years ago
Comment on attachment 8454253 [details] [diff] [review]
Remove all use of ScopedDeleteArray from mozglue/

Review of attachment 8454253 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozglue/linker/Mappable.cpp
@@ +90,4 @@
>      ERROR("Couldn't open %s to decompress library", path.get());
>      return nullptr;
>    }
> +  AutoUnlinkFile file(path.release());

The UniquePtr(UniquePtr<U, E>&&) constructor requires that the deleter type parameters be compatible:

    template<typename U, class E>
    UniquePtr(UniquePtr<U, E>&& other,
              typename EnableIf<IsConvertible<typename UniquePtr<U, E>::Pointer,
                                              Pointer>::value &&
                                !IsArray<U>::value &&
                                (IsReference<D>::value
                                 ? IsSame<D, E>::value
                                 : IsConvertible<E, D>::value),
                                int>::Type dummy = 0)

I didn't actually try it, but for D = MappableExtractFile::UnlinkFile and E = mozilla::DefaultDelete<char[]>, it's not the case that E converts to D, so just doing exactly what you suggest would fail.

This might be solved by adding a UnlinkFile(DefaultDelete<char[]>&&) constructor.  But that disables creation of the default constructor and so on for UnlinkFile, which seems likely to make things fall over.  And we don't have =default support to easily and (possibly) at no cost get the compiler back on the right track.

So overall, doing that seemed a bit of a rat's-nest of complexity best forgotten.

@@ +396,4 @@
>    if (!mappable->buffer)
>      return nullptr;
>  
> +  mappable->chunkAvail = MakeUnique<unsigned char[]>(mappable->zStream.GetChunksNum());

It doesn't need to.

MakeUnique for indeterminate-length array types calls new ArrayType[n]().  Per C++11 [expr.new]p15, the parens on the end of that call cause the resulting array (if successfully allocated) to be initialized as specified for direct initialization in [dcl.init].  The () initializer means value-initialized per [dcl.init]p10, which value-initializes each element ([dcl.init]p7), which zero-initializes for non-class types (same).

In contrast, if MakeUnique called new ArrayType[n] without parens, that would trigger default-initialize the array, which would default-initialize each element, which would default-construct classes (...approximately) and leave non-class elements uninitialized.  But MakeUnique doesn't actually do that.

I filed bug 1040830 on adding MakeUnique documentation which will document these semantics.
Assignee

Comment 10

5 years ago
Relanded with adjustments made, after some tryservering:

https://hg.mozilla.org/integration/mozilla-inbound/rev/ab90ae1ff611
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ba56adc0eef
Flags: needinfo?(jwalden+bmo)
Comment on attachment 8454252 [details] [diff] [review]
Remove all use of ScopedDeleteArray from media/

Review of attachment 8454252 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not too excited about the idioms here.

   UniquePtr<T[]> t_ = MakeUnique<T>(10);

Seems worse than:

   ScopedDeleteArray<T> t_(new T[10]);

In that you have to repeat T.


Also.

  foo_ = MakeUnique<T>() evocative

and would generally prefer foo_.reset(new T());

I'm prepared to hear arguments that the second of these is
just the new interim, but the first seems strictly worse.

::: media/mtransport/databuffer.h
@@ +33,4 @@
>    }
>  
>    void Assign(const uint8_t *data, size_t len) {
> +    data_ = MakeUnique<uint8_t[]>(len ? len : 1);  // Don't depend on new [0].

I would prefer data_.reset() here.

::: media/mtransport/nricectx.cpp
@@ +532,5 @@
>    if (stun_servers.empty())
>      return NS_OK;
>  
> +  UniquePtr<nr_ice_stun_server[]> servers =
> +    MakeUnique<nr_ice_stun_server[]>(stun_servers.size());

Can't you just initialize this, like so:


 UniquePtr<nr_ice_stun_server[]> servers(
      new nr_ice_stun_server[stun_servers.size()]);
Attachment #8454252 - Flags: review?(ekr) → review-
Assignee

Comment 13

5 years ago
(In reply to Eric Rescorla (:ekr) from comment #12)
>    UniquePtr<T[]> t_ = MakeUnique<T>(10);
> 
> Seems worse than:
> 
>    ScopedDeleteArray<T> t_(new T[10]);
> 
> In that you have to repeat T.

Aren't you repeating T in both?

An alternative would be to use |auto|, relying upon MakeUnique being so idiomatic that we can expect readers to intuitively grasp it (or at least be able to look it up quickly).

  auto t_ = MakeUnique<T[]>(10);

I'm fine with this, and in the long run it seems clearly best.  In these early days, I'm unsure about forcing the matter.  The JS patches I've landed *have* forced the matter, but outside JS I've taken it more slowly so far.

> Also.
> 
>   foo_ = MakeUnique<T>() evocative
> 
> and would generally prefer foo_.reset(new T());

Fine enough by me, I'd just think most people would want to see assignment over a member method call for changing the value of something.  I don't have a real preference for code that's not mine.  :-)

One reason I slightly prefer MakeUnique, is that it replaces an idiom that is only safe when used correctly -- new/new[] -- with one that is always safe, up until release() or a mismanaged get() occurs.  A stray MakeUnique that's dropped on the ground won't leak.  The same can't be said for new/new[].

> >    void Assign(const uint8_t *data, size_t len) {
> > +    data_ = MakeUnique<uint8_t[]>(len ? len : 1);  // Don't depend on new [0].
> 
> I would prefer data_.reset() here.

Fine enough, modulo MakeUnique being the safer idiom.

> Can't you just initialize this, like so:
> 
> 
>  UniquePtr<nr_ice_stun_server[]> servers(
>       new nr_ice_stun_server[stun_servers.size()]);

Certainly.  Just the general preference for safe idioms over potentially-unsafe ones in play here.
Flags: needinfo?(ekr)
OK. I'm sold. Change the local variables to auto and r=me

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #13)
> (In reply to Eric Rescorla (:ekr) from comment #12)
> >    UniquePtr<T[]> t_ = MakeUnique<T>(10);
> > 
> > Seems worse than:
> > 
> >    ScopedDeleteArray<T> t_(new T[10]);
> > 
> > In that you have to repeat T.
> 
> Aren't you repeating T in both?
> 
> An alternative would be to use |auto|, relying upon MakeUnique being so
> idiomatic that we can expect readers to intuitively grasp it (or at least be
> able to look it up quickly).
> 
>   auto t_ = MakeUnique<T[]>(10);
> 
> I'm fine with this, and in the long run it seems clearly best.  In these
> early days, I'm unsure about forcing the matter.  The JS patches I've landed
> *have* forced the matter, but outside JS I've taken it more slowly so far.
> 
> > Also.
> > 
> >   foo_ = MakeUnique<T>() evocative
> > 
> > and would generally prefer foo_.reset(new T());
> 
> Fine enough by me, I'd just think most people would want to see assignment
> over a member method call for changing the value of something.  I don't have
> a real preference for code that's not mine.  :-)
> 
> One reason I slightly prefer MakeUnique, is that it replaces an idiom that
> is only safe when used correctly -- new/new[] -- with one that is always
> safe, up until release() or a mismanaged get() occurs.  A stray MakeUnique
> that's dropped on the ground won't leak.  The same can't be said for
> new/new[].
> 
> > >    void Assign(const uint8_t *data, size_t len) {
> > > +    data_ = MakeUnique<uint8_t[]>(len ? len : 1);  // Don't depend on new [0].
> > 
> > I would prefer data_.reset() here.
> 
> Fine enough, modulo MakeUnique being the safer idiom.
> 
> > Can't you just initialize this, like so:
> > 
> > 
> >  UniquePtr<nr_ice_stun_server[]> servers(
> >       new nr_ice_stun_server[stun_servers.size()]);
> 
> Certainly.  Just the general preference for safe idioms over
> potentially-unsafe ones in play here.
Flags: needinfo?(ekr)
Assignee

Comment 15

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/7baced46f1c6 to remove the ~last use of ScopedDeleteArray.
Target Milestone: mozilla33 → ---
Depends on: 1467438
The leave-open keyword is there and there is no activity for 6 months.
:Waldo, maybe it's time to close this bug?
Flags: needinfo?(jwalden+bmo)
Assignee

Comment 19

3 months ago

No, it ain't time to close this, it's just it's not a high priority to do in any particular time frame.

I did a little fixing of this locally, and there are occasional trickinesses to it. :-|

For example, the .rwget() function in Scoped is used in places to expose a T* to pass to function calls, so the function can overwrite the owned contents. This isn't something UniquePtr supports or ever will support, and it gets very awkward fixing those, and there might not be a clean idiom to do it.

Some callers will also need to define a Deleter::Pointer type to store something other than T* inside them, and that's also messy to define.

Anyway, this should remain open, maybe I'll keep fixing things periodically, ideally patches would magically appear from the ether to fix this for me. :-)

Flags: needinfo?(jwalden)
Comment on attachment 9046524 [details] [diff] [review]
Replace a bunch of uses of Scoped with uses of UniquePtr in ICU-related code, and remove a couple pointless Scoped.h #includes

Review of attachment 9046524 [details] [diff] [review]:
-----------------------------------------------------------------

`NumberFormatDeleter` and `UniqueUNumberFormat` maybe could be moved into `ICUUtils.cpp`, because they're only used in that one file. But r+ with or without moving them.

::: intl/unicharutil/util/ICUUtils.h
@@ +16,5 @@
>  #  include "unicode/unum.h"  // for UNumberFormat
>  
>  class nsIContent;
>  
> +namespace mozilla {

I don't see where the `namespace` block is closed. Is it missing?
Attachment #9046524 - Flags: review?(andrebargull) → review+

Comment 21

3 months ago
Pushed by jwalden@mit.edu:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a0f04b2ce3e
Replace a bunch of uses of Scoped with uses of UniquePtr in ICU-related code, and remove a couple pointless Scoped.h #includes.  r=anba
You need to log in before you can comment on or make changes to this bug.