Page MenuHomePhabricator

Modernize: Use nullptr more.
Needs RevisionPublic

Authored by brucem on Feb 10 2018, 7:26 AM.

Details

Event Timeline

brucem created this revision.Feb 10 2018, 7:26 AM
brucem updated this revision to Diff 133778.Feb 10 2018, 5:17 PM

More nullptr usage.

dim added a subscriber: dim.Feb 11 2018, 11:53 AM

LGTM minus a few nits, though it would be nice if you can verify that all the changed headers still compile in -std=c++98 and/or -std=gnu++98 mode.

include/functional
1718

The cast can be removed now.

include/valarray
805

Missed __end here?

Is it worth adding -Werror=zero-as-null-pointer-constant to the build?

So my main concern with this patch is that nullptr is actually #defined'ed in C++03 mode. That definition comes from the __nullptr header, and therefore we would need to add that header to each include which uses it. Which kind of sucks.

dim added a comment.Feb 11 2018, 2:40 PM

So my main concern with this patch is that nullptr is actually #defined'ed in C++03 mode. That definition comes from the __nullptr header, and therefore we would need to add that header to each include which uses it. Which kind of sucks.

Indeed, but isn't nullptr used in many headers already? And as far as I can see, none of those includes <__nullptr> explicitly, so the definition must come from some transitive include.

brucem updated this revision to Diff 133996.Feb 12 2018, 10:51 PM

Addressed minor issues.

  • Addressed missing __end_ initialization from valarray.
  • Removed cast that was no longer needed.
  • Added nullptr usage to include/regex.
brucem marked 2 inline comments as done.Feb 12 2018, 10:53 PM
In D43159#1004639, @dim wrote:

So my main concern with this patch is that nullptr is actually #defined'ed in C++03 mode. That definition comes from the __nullptr header, and therefore we would need to add that header to each include which uses it. Which kind of sucks.

Indeed, but isn't nullptr used in many headers already? And as far as I can see, none of those includes <__nullptr> explicitly, so the definition must come from some transitive include.

It is handled in the wrapper around stddef.h so it all should just work.

Is it worth adding -Werror=zero-as-null-pointer-constant to the build?

I'll look at this as a follow up.

brucem added a comment.Jun 1 2019, 4:47 AM

Can we revive this review? I'd still like to land this ...

Can we revive this review? I'd still like to land this ...

What was the result of testing with -std=c++98 and/or -std=gnu++98 ?
The code changes look fine; but as @EricWF said....

brucem updated this revision to Diff 202554.Jun 1 2019, 8:26 AM

Updated to current master and added more nullptr usage.

brucem updated this revision to Diff 202555.Jun 1 2019, 8:28 AM

Remove CMakeLists.txt change.

zoecarver added inline comments.
include/memory
1259

This could be nullptr too.

brucem updated this revision to Diff 202562.Jun 1 2019, 10:18 AM

More nullptr-ification.

brucem marked 2 inline comments as done.Jun 1 2019, 10:22 AM
brucem added inline comments.
include/memory
1259

This pointed out a number of things that could be fixed, so did that and updated.

brucem marked an inline comment as done.Jun 1 2019, 10:43 AM

What was the result of testing with -std=c++98 and/or -std=gnu++98 ?
The code changes look fine; but as @EricWF said....

It seems to work for me in -std=c++98 ...

I responded to the point that @EricWF made before ... the __nullptr header is included by the wrapper around stddef.h ... and these aren't the first / only usages of nullptr in the headers.

brucem updated this revision to Diff 207356.Jul 1 2019, 9:32 AM

Updating to current master.

brucem added a comment.Jul 1 2019, 9:33 AM

In addition to my previous explanation for the concerns about using nullptr more ... it should be noted that many of these files already use nullptr.

ldionne accepted this revision.Jul 2 2019, 9:31 AM

I'm fine with this given what the author said about nullptr already being used in those headers.

This revision is now accepted and ready to land.Jul 2 2019, 9:31 AM
mclow.lists requested changes to this revision.Jul 3 2019, 8:29 AM

Did you try to build libc++ or run the tests before submitting this?

include/__threading_support
323

This one is wrong.

This revision now requires changes to proceed.Jul 3 2019, 8:29 AM

In <algorithm>, you missed a couple of (value_type*)0. Line 3356, 3369, 3486 and 3499.

mclow.lists added inline comments.Jul 3 2019, 8:41 AM
include/algorithm
4431

I'm not really a fan of casting nullptr to a value_type *. This change doesn't seem to add anything to the code; it's just longer.

The call to __incr doesn't use the pointer at all. Maybe a better approach is to change

template <class _Tp>
_LIBCPP_INLINE_VISIBILITY void __incr(_Tp*) _NOEXCEPT
    {__incr(integral_constant<bool, is_trivially_destructible<_Tp>::value>());}

to:

template <class _Tp>
_LIBCPP_INLINE_VISIBILITY void __incr() _NOEXCEPT
    {__incr(integral_constant<bool, is_trivially_destructible<_Tp>::value>());}

and remove the nullptrs from the calls in <algorithm>
Alternately, add an overload of __incr that takes a nullptr_t.

ldionne requested changes to this revision.Jul 3 2019, 12:22 PM

I agree with Marshall's requests.

mclow.lists added inline comments.Jul 3 2019, 1:08 PM
include/__threading_support
323

__libcpp_thread_t is an alias for an operating-system specific type.
On Mac OS, it is a pointer to some Darwin-specific type.
On Ubuntu, it is a const unsigned long.

You can't compare it to nullptr.

ldionne added inline comments.Jul 4 2019, 9:54 AM
include/__threading_support
323

I think the comparison should be *__t == __libcpp_thread_t().