Closed Bug 1120062 Opened 5 years ago Closed 5 years ago

Remove MOZ_HAVE_CXX11_NULLPTR and NullptrT because we no longer need to emulate nullptr

Categories

(Core :: MFBT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: cpeterson, Assigned: emk)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Attachment #8547052 - Flags: review?(jwalden+bmo)
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.)
Attachment #8547052 - Flags: review?(jwalden+bmo) → review+
Thanks. Landing everything other than NullPtr.h and UniquePtr.h.
https://hg.mozilla.org/integration/mozilla-inbound/rev/3928ee1b0381
Keywords: leave-open
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?
Flags: needinfo?(jwalden+bmo)
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.
Flags: needinfo?(jwalden+bmo)
Thanks for the advice.
Try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=35dac4fa6a50
Attachment #8547129 - Attachment is obsolete: true
Attachment #8548459 - Flags: review?(jwalden+bmo)
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.
Attachment #8548459 - Flags: review?(jwalden+bmo) → review+
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/e5fee301e66c
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.