1164602 - Replace NullPtr hack with nullptr_t

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Terrence Cole [:terrence]]

Attachment: use_nullptr_t_instead_of_NullPtr-v0.diff

When we implemented Handle, nullptr_t was still a typedef to 0 on some platforms. Now that it is a real thing everywhere, we should be able to replace our NullPtr workaround with a constructor taking std::nullptr_t. The only remaining issue is that some platforms do not have a modern libstdc++, so do not have std::nullptr_t. Luckily we can get around that by using decltype(nullptr) instead. This patch appears to work locally; try run is below to check the other platforms:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b845f5801f5

The attached patch was generated with:
$ find . -name "*.cpp" -o -name "*.h" -exec sed -i 's/JS::NullPtr()/nullptr/g' \{\} \;
$ find . -name "*.cpp" -o -name "*.h" -exec sed -i 's/js::NullPtr()/nullptr/g' \{\} \;
$ find . -name "*.cpp" -o -name "*.h" -exec sed -i 's/NullPtr()/nullptr/g' \{\} \;

With a bit of hand tweaking afterwards to fix up Codegen.py and JS::GCCellPtr::NullPtr.

Comment 1

[Terrence Cole [:terrence]]

Attachment: use_nullptr_t_instead_of_NullPtr-v1.diff

Whoops! Forgot that the null needs to be at the second level of indirection. Easy enough to solve.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a53f9c3cec0b

Comment 2

[Terrence Cole [:terrence]]

Try run looks green.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a53f9c3cec0b

Comment 3

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8605513 [details] [diff] [review]
use_nullptr_t_instead_of_NullPtr-v1.diff

Review of attachment 8605513 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/public/RootingAPI.h
@@ +120,5 @@
>  
>  template <typename T>
>  class PersistentRootedBase {};
>  
> +static void*const ConstNullValue = nullptr;

static void* const ConstNullValue = nullptr;

assuming this is still needed?

@@ +407,3 @@
>          static_assert(mozilla::IsPointer<T>::value,
> +                      "nullptr_t overload not valid for non-pointer types");
> +        ptr = reinterpret_cast<const T*>(&js::ConstNullValue);

Wouldn't this be better with just a static nullptr declared right here? Or do we compare these pointers somehow?

Comment 4

[Terrence Cole [:terrence]]

InternalHandle's constructor also needed an &nullptr and I thought it would be simpler to share them. I mean, it would be weird for null handle != null handle, even if we never actually use the feature.

Comment 5

[Terrence Cole [:terrence]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a53f9c3cec0b

Comment 6

[Terrence Cole [:terrence]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a53f9c3cec0b

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0deb2843004f

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0deb2843004f