Closed Bug 1410252 Opened 7 years ago Closed 7 years ago

Add MakeNotNull to construct NotNull from infallible new

Categories

(Core :: MFBT, enhancement)

Product:
Core
Component:
MFBT
Version:
58 Branch
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: mozbugz, Assigned: mozbugz)

References

Details

Attachments

(2 files)

Bug 1410252 - MakeNotNull<PointerType, OptionalPointeeType>(Args...) -
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1410252 - Convert 'WrapNotNull(new T(...' to 'MakeNotNull<T*>(...' -
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details 
Currently the only way to construct a `NotNull` is through `WrapNotNull`, which takes a pointer, but *always* checks it for null with a MOZ_RELEASE_ASSERT.
That check is unnecessary in lots of places when allocating objects with non-fallible `new`.

I'm proposing we add `MakeNotNull`, in the style of `MakeUnique`, which will remove the need for a null check.

Also, removing naked `new`s is usually considered A Good Thing anyway.

MakeNotNull<T*> should be trivial enough.
MakeNotNull<RefPtr<T>> (and other smart pointers) may just work too, but this should be tested.
Assignee: nobody → gsquelart
Comment on attachment 8920837 [details]
Bug 1410252 - MakeNotNull<PointerType, OptionalPointeeType>(Args...) -

https://reviewboard.mozilla.org/r/191824/#review197002


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: mfbt/tests/TestNotNull.cpp:327
(Diff revision 1)
> +  delete nnci;
> +
> +  // Create a derived object and store its base pointer.
> +  struct Base
> +  {
> +    virtual ~Base(){};

Warning: Use '= default' to define a trivial destructor [clang-tidy: modernize-use-equals-default]

    virtual ~Base(){};
            ^
                   = default;
Comment on attachment 8920838 [details]
Bug 1410252 - Convert 'WrapNotNull(new T(...' to 'MakeNotNull<T*>(...' -

https://reviewboard.mozilla.org/r/191826/#review197018
Attachment #8920838 - Flags: review?(n.nethercote) → review+
Comment on attachment 8920837 [details]
Bug 1410252 - MakeNotNull<PointerType, OptionalPointeeType>(Args...) -

https://reviewboard.mozilla.org/r/191824/#review197016

::: mfbt/NotNull.h:167
(Diff revision 1)
> +NotNull<T>
> +MakeNotNull(Args&&... aArgs)
> +{
> +  // Extract the pointee type from what T's dereferencing operator returns
> +  // (which could be a reference to a const type).
> +  using O = typename mozilla::RemoveConst<

Is O a typical name for this sort of thing? It's hard to read, because it looks like 0. Maybe use U instead?

::: mfbt/tests/TestNotNull.cpp:353
(Diff revision 1)
> +  static_assert(mozilla::IsSame<NotNull<MyPtr<int>>, decltype(nnmi)>::value,
> +                "MakeNotNull<MyPtr<int>> should return NotNull<MyPtr<int>>");
> +  CHECK(*nnmi == 23);
> +  delete nnmi.get().get();
> +
> +  auto nnui = MakeNotNull<UniquePtr<int>>(24);

I'm surprised by this because I thought that UniquePtr and NotNull were incompatible:
http://searchfox.org/mozilla-central/source/mfbt/NotNull.h#98-99
Attachment #8920837 - Flags: review?(n.nethercote) → review+
Comment on attachment 8920837 [details]
Bug 1410252 - MakeNotNull<PointerType, OptionalPointeeType>(Args...) -

https://reviewboard.mozilla.org/r/191824/#review197016

> Is O a typical name for this sort of thing? It's hard to read, because it looks like 0. Maybe use U instead?

Not a typical name, but I see that it's hard to read. I'll just go with 'Pointee' as it's more readable and unambiguous.

> I'm surprised by this because I thought that UniquePtr and NotNull were incompatible:
> http://searchfox.org/mozilla-central/source/mfbt/NotNull.h#98-99

Well, you were more daring that GSL programmers:
If you look at that discussion, the main issue is that unique_ptr cannot be copy-constructed, but the official gsl::not_null can *only* be copy-constructed.
But yours has a move constructor, so move-only types can be used.

Also, for my particular code, I'm actually creating a `int*` (extracted from `UniquePtr<int>`), and passing that raw pointer to `NotNull<UniquePtr<int>>::NotNull(int*)`, and that raw pointer is passed to `UniquePtr<int>::UniquePtr(Pointer)` -- All allowed and well.

I'll open a separate bug to review this (I'd say just remove the comment, and add a few more tests to prove that it works.)
Pushed by gsquelart@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ec8754342714
MakeNotNull<PointerType, OptionalPointeeType>(Args...) - r=njn
https://hg.mozilla.org/integration/autoland/rev/5fbd0369b400
Convert 'WrapNotNull(new T(...' to 'MakeNotNull<T*>(...' - r=njn
Huh, interesting. I'm not much of a C++ language lawyer, I probably shouldn't have been the one to write this code :)
Thank you for the review.

I've opened bug 1410760 to review this NotNull<UniquePtr<T>> comment. (I can't work on it now.)

I think you did a good job with NotNull. I'm guessing you didn't just copy-paste gsl::not_null, and that's why you went with what seemed natural (both copy and move).
The GSL folks were more cautious and only considered not_null being used with raw pointers. I think we should definitely see what they are going to do (in case they do find issues). In the meantime, hopefully bug 1410760 will prove that it works for us; and if not, we can remove the move special members.
"Pointee" seems like an odd term to my ears.  Perhaps "Target" would be better?  (I don't see "pointee" in https://en.wikipedia.org/wiki/Pointer_(computer_programming) and I see a *few* occurrences of "target", but maybe there's something better... maybe a term related to type polymorphism?)
I'm used to reading&hearing "pointee", so I always assumed it was the obvious word for what a "pointer" points to.
(But being a non-native English speaker, I may have picked from the wrong source!)
"Target" sounds fine to me. I've also seen a more verbose "pointed-to type".

Anyway, this patch has landed, and this word is only used inside one function, so I'll copy your comment in follow-up bug 1410760, it should be easy to add a correction there.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: