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.

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

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

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.

https://hg.mozilla.org/mozilla-central/rev/ec8754342714 https://hg.mozilla.org/mozilla-central/rev/5fbd0369b400