Improve usability of OwningNonNull

RESOLVED FIXED in Firefox 40

Status

()

Core
DOM
--
enhancement
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: ayg, Assigned: ayg)

Tracking

unspecified
mozilla40
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(3 attachments, 3 obsolete attachments)

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).
Flags: in-testsuite-
Created attachment 8591368 [details] [diff] [review]
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!
Flags: needinfo?(bzbarsky)
Created attachment 8591370 [details] [diff] [review]
Part 1 - Improve usability of OwningNonNull

Uploaded wrong file.
Attachment #8591368 - Attachment is obsolete: true
Created attachment 8591371 [details] [diff] [review]
Part 2 - Use some OwningNonNull in editor
Attachment #8591371 - Flags: review?(ehsan)
(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);
  }
Attachment #8591370 - Attachment is obsolete: true
Attachment #8591371 - Attachment is obsolete: true
Attachment #8591371 - Flags: review?(ehsan)
> 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.
Flags: needinfo?(bzbarsky)
Created attachment 8591723 [details] [diff] [review]
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.
Attachment #8591723 - Flags: review?(bzbarsky)
Created attachment 8591725 [details] [diff] [review]
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.
Attachment #8591725 - Flags: review?(ehsan)
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
Attachment #8591723 - Flags: review?(bzbarsky) → review+
(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()?
Flags: needinfo?(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...
Flags: needinfo?(bzbarsky)
Created attachment 8593433 [details] [diff] [review]
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.
Attachment #8593433 - Flags: review?(bzbarsky)
Comment on attachment 8593433 [details] [diff] [review]
Part 3 - More OwningNonNull improvements

r=me
Attachment #8593433 - Flags: review?(bzbarsky) → review+

Updated

3 years ago
Attachment #8591725 - Flags: review?(ehsan) → review+
https://hg.mozilla.org/mozilla-central/rev/d14894a05d88
https://hg.mozilla.org/mozilla-central/rev/660aac389b8f
https://hg.mozilla.org/mozilla-central/rev/a77621e815fc
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Depends on: 1157848
You need to log in before you can comment on or make changes to this bug.