1153649 - Improve usability of OwningNonNull

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement)

Description

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

We want to use this in editor, but it needs some added features to be usable (like being able to initialize it to a non-null value).

Comment 1

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 1 - Improve usability of OwningNonNull

bz, if you aren't doing reviews right now, do you have suggestions for who should review instead?  Or should it just wait?


This comes from trying to use it in a few places in editor (see next patch) and fixing the errors.  Notes:

* I deliberately didn't add a constructor from a pointer, only a reference, so that users with a pointer would have to explicitly dereference to indicate they're assuming it's non-null.  (As discussed by private e-mail, I would like to work out a way to remove the operator=(T*) also.)

* I added the constructor from already_AddRefed&&, but I don't like it much, because like constructing from a pointer, it hides the assumption that the value is non-null.  We don't have any sort of replacement, though.  I would like functions to return OwningNonNull&&, but if people don't like that, I guess we need a new class.

* I don't think the existing assignment operator from already_AddRefed actually works, for the record.

* I like the idea of deleting operator bool, to catch unreachable code paths.  I'm not sure this is the right way to do it, though.

* This is missing a swap() method, which can sometimes save an addref/release.  But I don't think it's essential to add right now -- too many permutations!

Comment 2

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 1 - Improve usability of OwningNonNull

Uploaded wrong file.

Comment 3

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 2 - Use some OwningNonNull in editor

Comment 4

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to :Aryeh Gregor from comment #1)
> * This is missing a swap() method, which can sometimes save an
> addref/release.  But I don't think it's essential to add right now -- too
> many permutations!

On second thought, it would be easy -- something like

  template<typename U>
  void
  swap(U aOther)
  {
    mPtr.swap(aOther);
  }

Comment 5

[Boris Zbarsky [:bzbarsky]]

> bz, if you aren't doing reviews right now

I can do this review.  I don't expect it to be particularly time-consuming.

>  swap(U aOther)

U&, yes?  Otherwise you pass-by-value, swap into the temp value, and bad things happen, I would think.

Comment 6

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 1 - Improve usability of OwningNonNull

Further notes on new version:

* I marked the constructors MOZ_IMPLICIT to stop static analysis warnings, based on nsCOMPtr/nsRefPtr.

* I made the bool operator explicit to stop some compile errors I got when trying to use it (nsTArray.IndexOf didn't know whether to compare as T* or int).

* Added swap method as discussed.

Comment 7

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 2 - Use some OwningNonNull in editor

Lots more in future patches.  This is pretty much a PoC.  Note that I still had to remove the .swap(), because nsRefPtr doesn't currently swap with nsCOMPtr.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8591723 [details] [diff] [review]
Part 1 - Improve usability of OwningNonNull

>+  operator->() const
>+    MOZ_ASSERT(mPtr, "OwningNonNull<T> was set to null");

Why is that needed?  If mInited is true, then the assert happened at the point when the set/init happened, right?

On the other hand, it may be worth it to assert &aValue in the ctor taking a ref, in case someone passes *nullptr in and it doesn't just crash at the deref site or something.

r=me with that

Comment 9

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to Not doing reviews right now from comment #8)
> Why is that needed?  If mInited is true, then the assert happened at the
> point when the set/init happened, right?

I just copied from operator T*.  Do you want me to get rid of that assert too?

> On the other hand, it may be worth it to assert &aValue in the ctor taking a
> ref, in case someone passes *nullptr in and it doesn't just crash at the
> deref site or something.

Isn't this already covered by the assert in init()?

Comment 10

[Boris Zbarsky [:bzbarsky]]

> I just copied from operator T*.  Do you want me to get rid of that assert too?

Hmm...  Let's just leave both for now; I'll try to think about why we added it in operator T*.

> Isn't this already covered by the assert in init()?

So it is.  I'm clearly not thinking straight...

Comment 11

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Green try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c8bc8694cf2

Comment 12

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 3 - More OwningNonNull improvements

Compile-tested locally, with some testing to make sure the changes actually do what I want.  I didn't submit to try yet.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8593433 [details] [diff] [review]
Part 3 - More OwningNonNull improvements

r=me

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d14894a05d88
https://hg.mozilla.org/integration/mozilla-inbound/rev/660aac389b8f
https://hg.mozilla.org/integration/mozilla-inbound/rev/a77621e815fc

Comment 15

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Green try including part 3: https://treeherder.mozilla.org/#/jobs?repo=try&revision=900a21d93e6f

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/d14894a05d88
https://hg.mozilla.org/mozilla-central/rev/660aac389b8f
https://hg.mozilla.org/mozilla-central/rev/a77621e815fc