This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Replace several uses of 0 by nullptr
ClosedPublic

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

Details

Reviewers
mclow.lists
EricWF
brucem
ldionne
curdeius
Group Reviewers
Restricted Project
Restricted Project
Commits
rG527a7fdfbd74: [libc++] Replace several uses of 0 by nullptr

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().

Ping -- is there still interest in moving forward with this? If so, please address the comments and update the patch, otherwise you can abandon the revision to clean up the libc++ review queue.

ldionne commandeered this revision.Nov 24 2020, 9:57 AM
ldionne edited reviewers, added: brucem; removed: ldionne.
ldionne updated this revision to Diff 307398.Nov 24 2020, 9:57 AM

Rebase onto master

Herald added a project: Restricted Project. · View Herald TranscriptNov 24 2020, 9:57 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne retitled this revision from Modernize: Use nullptr more. to [libc++] Replace several uses of 0 by nullptr.Nov 24 2020, 9:57 AM
ldionne updated this revision to Diff 307402.Nov 24 2020, 10:08 AM

Address review comments.

Herald added a project: Restricted Project. · View Herald TranscriptNov 24 2020, 10:08 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne accepted this revision.Nov 24 2020, 10:08 AM

I'll ship this if CI passes, since I addressed all comments we had originally.

curdeius added inline comments.
libcxx/include/memory
4371 ↗(On Diff #307402)

Unneeded chang for use_count(). It's a long.

curdeius added inline comments.Nov 26 2020, 6:36 AM
libcxx/include/functional
1759 ↗(On Diff #307402)

All uses of __f_ should also use nullptr. If my search counted correctly, there are 15 of them in __value_func.

libcxx/include/locale
4335 ↗(On Diff #307402)

You missed this one.

4340 ↗(On Diff #307402)

And here.

ldionne updated this revision to Diff 307888.Nov 26 2020, 8:58 AM
ldionne marked 3 inline comments as done.

Apply review comments. Thanks for the catches!

This revision was not accepted when it landed; it landed in state Needs Review.Nov 27 2020, 7:00 AM
This revision was automatically updated to reflect the committed changes.