Open Bug 1055035 Opened 6 years ago Updated 6 years ago

remove nsAutoRef.h, converting clients to UniquePtr/nsRefPtr as appropriate

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

People

(Reporter: froydnj, Unassigned)

References

Details

nsAutoRef.h contains the odd beasts nsAutoRef and nsCountedRef.  The former emulates nsAutoPtr, but with a customizable free() operation.  The latter emulates nsRefPtr, but with customizable AddRef/Release (so that one might easily use it with third-party reference counting, for instance).  I find them somewhat poorly named, poorly integrated with useful features (e.g. move constructors, leak checking), using nsAutoRefTraits for *both* things is sort of odd (since you implement the traits class differently depending on the actual pointer container you want to use), and we have other, more widely-used things in the tree that they can be replaced with.

The former can be emulated with UniquePtr.

The latter, I think, requires an extra level of indirection to replace with nsRefPtr, which is unfortunate.  Unless we want to implement something like bug 463337 instead and move a lot of the leak-checking machinery in nsRefPtr over to nsCountedRef...We can see how loudly people yell when shuffling things around.
Depends on: 1055114
Depends on: 1055141
UniquePtr uses DefaultDelete<T> by default which uses delete or delete[] unless specialized.

I assume that means that a client using UniquePtr<T> that doesn't include a header specializing DefaultDelete<T> will call delete on the Ptr, even if a specialization is provided in a different compilation unit.  (I assume the linker doesn't resolve and inline specializations undeclared in a particular compilation unit.)
If so, that sounds fragile wrt header includes if delete should not be called on the pointer and a specialization is intended.

I assume the intention is that UniquePtr's template parameter D should be provided explicitly if delete is not the appropriate way to dispose of the object?

I wonder whether there is a way to prevent specialization of DefaultDelete.
Flags: needinfo?(nfroyd)
Flags: needinfo?(jwalden+bmo)
(In reply to Karl Tomlinson (:karlt) from comment #1)
> I assume that means that a client using UniquePtr<T> that doesn't include a
> header specializing DefaultDelete<T> will call delete on the Ptr, even if a
> specialization is provided in a different compilation unit.  (I assume the
> linker doesn't resolve and inline specializations undeclared in a particular
> compilation unit.)

This assumption is correct; the linker can't even do this (ignoring PGO, and even then...).

> If so, that sounds fragile wrt header includes if delete should not be
> called on the pointer and a specialization is intended.

It is indeed tedious and somewhat fragile, as adventures in removing nsAutoRef have proven.  The method provided by nsAutoRefTraits seems a little safer in this respect.

Would it be a bad thing if we made mozilla::UniquePtr closer to nsAutoRef's behavior in this respect, rather than the standard's?  (nsAutoRef's behavior is also more convenient for dealing with legacy types from e.g. C libraries.)

> I assume the intention is that UniquePtr's template parameter D should be
> provided explicitly if delete is not the appropriate way to dispose of the
> object?

It's not obvious to me whether one is supposed to explicitly provide the template parameter D or simply specialize DefaultDelete for the appropriate type.  AFAICS, the standard supplies no advice on this issue, which leads me to believe that both ways are intended to be used.

> I wonder whether there is a way to prevent specialization of DefaultDelete.

I don't think you can do this.
Flags: needinfo?(nfroyd)
(In reply to Nathan Froyd (:froydnj) from comment #2)
> Would it be a bad thing if we made mozilla::UniquePtr closer to nsAutoRef's
> behavior in this respect, rather than the standard's?

If you mean by not providing the delete default template implementation for
DefaultDelete, then that would be safer.

However, I'd like to see nsAutoPtr also replaced by UniquePtr and providing
specializations for every deleted class would get tedious.

> (nsAutoRef's behavior
> is also more convenient for dealing with legacy types from e.g. C libraries.)

I haven't used UniquePtr enough to know what you refer to here.

> > I assume the intention is that UniquePtr's template parameter D should be
> > provided explicitly if delete is not the appropriate way to dispose of the
> > object?
> 
> It's not obvious to me whether one is supposed to explicitly provide the
> template parameter D or simply specialize DefaultDelete for the appropriate
> type.  AFAICS, the standard supplies no advice on this issue, which leads me
> to believe that both ways are intended to be used.

Yes, I don't see documentation indicating intended use.

However, a specialized DefaultDelete is no longer the "default", so perhaps
the naming implies that specialization is not intended.

If we want safe behavior with a custom deletion operation, then two options
are:

1) Provide the D template parameter explicitly.

   This makes declarations verbose, but perhaps that can be mitigated with
   typedefs and a convention for the typedef names.

2) Have separate classes, but with similar names, one providing delete, and
   the other requiring a specialization.

I noticed that the specializations of nsAutoRefTraits were getting hard to use
in headers [1] and I suspect the same issues apply to DefaultDelete, so I'm hopeful that the D template parameter to UniquePtr might provide a nicer solution.

[1] Thing 2 in https://bugzilla.mozilla.org/show_bug.cgi?id=497498#c38
(In reply to Karl Tomlinson (:karlt) from comment #1)
> I assume that means that a client using UniquePtr<T> that doesn't include a
> header specializing DefaultDelete<T> will call delete on the Ptr, even if a
> specialization is provided in a different compilation unit.  (I assume the
> linker doesn't resolve and inline specializations undeclared in a particular
> compilation unit.)
> If so, that sounds fragile wrt header includes if delete should not be
> called on the pointer and a specialization is intended.

I think this is one of those "no diagnostic is required" sorts of mistakes people can make, tho I couldn't cite chapter and verse on this.  That said, specializing *after* use of the unspecialized version is, I think, guaranteed to make the program not well-formed and hits the infamous "self-immolation" note in C++11.

You can't prevent someone specializing a template, other than by specializing it before they can.

> I assume the intention is that UniquePtr's template parameter D should be
> provided explicitly if delete is not the appropriate way to dispose of the
> object?
> 
> I wonder whether there is a way to prevent specialization of DefaultDelete.

Specializing std::default_delete<T> is fine *if* T is or depends upon a user-defined type, per  [namespace.std]p2.  For a type that is *always* deleted the same way (not using delete or delete[]), specializing mozilla::DefaultDelete<T> is reasonable.  Such specialization should occur immediately after the definition of T.  That addresses the header-ordering fragility.

But, for many things that want non-default deletion, the deletion policy isn't purely determined by the type but rather depends on the particular mode of creation.  Think low-level C-style allocator mismatches.  Those cases want different deletion policies, not specialization of DefaultDelete.

Re legacy types, I think it's better for us to transition away from such users, than to change UniquePtr (and abandon any hope of using std::unique_ptr in the future) to accommodate them.  We shouldn't accommodate them as an exceptional case, but rather we should make them change.  And the boilerplate to deal with them with custom deletion policies isn't that bad if they refuse to switch.
Flags: needinfo?(jwalden+bmo)
You need to log in before you can comment on or make changes to this bug.