MSVC 2013 and gcc 4.6 are now the minimum supported versions and they both support nullptr natively. We should be able to remove MOZ_HAVE_CXX11_NULLPTR and NullptrT for emulating nullptr.

Comment on attachment 8547052 [details] [diff] [review] Remove MOZ_HAVE_CXX11_NULLPTR and NullptrT Review of attachment 8547052 [details] [diff] [review]: ----------------------------------------------------------------- r+ on everything but Nullptr.h and UniquePtr.h (and because of what you're doing, all that can land now, without waiting on the stragglers). I'd like to take a look at the updated versions of both of those files. IsNullPointer was added mostly for completeness, tho it's come in handy in those cases trying to implement nullptr overloads. We need to review all those uses and see which can be simplified. More patches (or patch, looks like there are few enough) for those uses, please? mfbt/Move.h has some obsolete comments about how nullptr can't be perfectly forwarded, that also need to be updated. I have vague memory of at least one more place that needs changing, but I'm not finding it right now. Maybe it'll come to me before you come back with more patches here. ::: mfbt/NullPtr.h @@ +6,5 @@ > > /* > + * Ideally this would be in TypeTraits.h, but C++11 omitted std::is_null_pointer > + * (fixed in C++14), so in the interests of easing a switch to <type_traits>, > + * this trait lives elsewhere. This position is where a brief file-overview comment goes. This detail doesn't belong here. Nonetheless the comment does need to be rewritten now, so let's go with this: /* Implements a mozilla::IsNullPointer<T> type trait. */ @@ +16,4 @@ > namespace mozilla { > > /** > + * IsNullPointer<T>::value is true iff T is the type of C++11's nullptr. ...iff T is decltype(nullptr). @@ +20,4 @@ > * > + * IsNullPointer is useful to give an argument the decltype(nullptr) type. > + * Some examples (that assume namespace mozilla has been opened, for > + * simplicity): Everything that follows here is more or less ridiculous amounts of hairiness to deal with decltype(nullptr) not always being C++11's std::nullptr_t. * // foo(T*), foo(stuff that converts to T*), and foo(decltype(nullptr)) * // (only invoked if nullptr is true nullptr, otherwise foo(T*) is invoked) * // but nothing else * void foo(T*) { } * template<typename N> * void foo(N, * typename EnableIf<IsNullPointer<N>::value, int>::Type dummy = 0) * { } in a world with true nullptr is just void foo(T*) { } void foo(decltype(nullptr)) { } * // foo(T*) *exactly* and foo(decltype(nullptr)), nothing else * void foo(T*) { } * template<typename U> * void foo(U, * typename EnableIf<!IsNullPointer<U>::value, int>::Type dummy = 0) * = delete; in a world with true nullptr is just void foo(T*) { } void foo(decltype(nullptr)) { } template<typename U> void foo(U) = delete; These simpler bits of code are not entirely trivial. But anyone with some understanding (possibly even basic understanding) of how overloading and template overloading interact could figure it out. It's probably also something that could be found in web searches and on Stack Overflow. In contrast very few people ever needed to understand the gory details of our nullptr emulation (and I wrote these comments to actively discourage it!), writing correct overload code respecting a type that could be either one type or two types was much harder (particularly in the presence of compiler warnings if you tried to use decltype(__null) to target nullptr), and it was nigh-impossible to test any such code without multiple try-server iterations. This *all* derived from nastiness in this file. None of it has any relevance when nullptr is really nullptr. So you can trim down the whole comment to just this: /** * IsNullPointer<T>::value is true iff T is decltype(nullptr). * * Ideally this would be in TypeTraits.h, but C++11 omitted std::is_null_pointer * (fixed in C++1y), so in the interests of easing a switch to <type_traits>, * this trait lives elsewhere. */ @@ +44,3 @@ > */ > template<typename T> > struct IsNullPointer { static const bool value = false; }; We weren't using mozilla::FalseType here because we didn't want this header silently introducing anything more than it had to, to make it easier to remove. Now that it's providing only this trait, and this trait may well be in enough STLs soon to use <type_traits> soon enough, we can use #include "mozilla/TypeTraits.h" for this: template<typename T> struct IsNullPointer : FalseType {}; template<> struct IsNullPointer<decltype(nullptr)> : TrueType {}; @@ +48,3 @@ > template<> > struct IsNullPointer<decltype(nullptr)> { static const bool value = true; }; > } Blank line before closing } of a namespace block. ::: mfbt/UniquePtr.h @@ +279,2 @@ > { > MOZ_ASSERT(aNull == nullptr); Remove this assert, too. @@ +280,5 @@ > MOZ_ASSERT(aNull == nullptr); > reset(nullptr); > return *this; > } > In C++11/14, I believe UniquePtr<T[], D>::reset(decltype(nullptr)) is a separate method from UniquePtr<T[], D>::reset(Pointer p = Pointer()). So you should add that new reset method to the T[] specialization. (I'm not 100% sure off the top of my head if this distinction is observable -- I think it is, with pointer to member function shenanigans, if probably not in normal code. Still, shouldn't let a difference keep in if we can help it.) @@ +556,2 @@ > { > MOZ_ASSERT(aNull == nullptr); Remove the assertion, and I'd say remove " aNull" since there's no reason to name a nullptr argument as there's only one possible value std::nullptr_t can be. Same for all the rest of these. ::: parser/html/jArray.h @@ +95,2 @@ > // Make assigning null to an array in Java delete the buffer in C++ > MOZ_ASSERT(n == nullptr); Remove this MOZ_ASSERT, it's always true now. (It wasn't before in that NullptrT could have been void* and could have been non-null, in theory -- if only in code compiled by gcc < 4.6.)

Thanks. Landing everything other than NullPtr.h and UniquePtr.h. https://hg.mozilla.org/integration/mozilla-inbound/rev/3928ee1b0381

I tried to replace IsNullPointer with overloads, but Linux x86-64 debug static analysis build fails: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e2d2312a7f9 > ../../../dist/include/mozilla/UniquePtr.h:232:3: error: bad implicit conversion constructor for 'UniquePtr' > ../../../dist/include/mozilla/UniquePtr.h:412:3: error: bad implicit conversion constructor for 'UniquePtr' What's wrong?

That happens when you have a non-explicit constructor without MOZ_IMPLICIT on it. It should be implicit because C++11/14 say it is, so smack that on it.

Thanks for the advice. Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=35dac4fa6a50

Comment on attachment 8548459 [details] [diff] [review] Part 2: Remove use of IsNullPointer Review of attachment 8548459 [details] [diff] [review]: ----------------------------------------------------------------- ::: mfbt/Move.h @@ -195,5 @@ > * int tmp = s.x; > * C(tmp, 0); // OK: tmp not a bit-field > - * > - * The legacy issue is that when we don't have true nullptr and must emulate > - * it (gcc 4.4/4.5), forwarding |nullptr| results in an |int| or |long| You also need to change the "There are two issues" language above this. ::: mfbt/UniquePtr.h @@ +234,1 @@ > : mTuple(static_cast<Pointer>(nullptr), DeleterType()) I'm about 99% sure this static_cast can be removed now -- it existed purely to deal with imperfect forwarding of nullptr at the time. @@ +415,1 @@ > : mTuple(static_cast<Pointer>(nullptr), DeleterType()) Same here. @@ +650,1 @@ > * An exception thrown after |new T| succeeds will leak that memory, unless the About 25 lines up you need to delete the "literal nullptr must not be provided as an argument" parenthetical. ::: mfbt/tests/TestUniquePtr.cpp @@ +383,2 @@ > template<typename T> > +struct AppendNullptrTwice I no longer quite remember the why for this code, exactly -- maybe to test appending of nullptr works in compilers that support it, even if at the time it was generally useless, as future-proofing. Meh.