Details
- Reviewers
mclow.lists EricWF brucem ldionne curdeius - Group Reviewers
Restricted Project Restricted Project - Commits
- rG527a7fdfbd74: [libc++] Replace several uses of 0 by nullptr
Diff Detail
- Repository
- rCXX libc++
- Build Status
Buildable 34141 Build 34140: arc lint + arc unit
Event Timeline
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.
Addressed minor issues.
- Addressed missing __end_ initialization from valarray.
- Removed cast that was no longer needed.
- Added nullptr usage to include/regex.
What was the result of testing with -std=c++98 and/or -std=gnu++98 ?
The code changes look fine; but as @EricWF said....
include/memory | ||
---|---|---|
1259 | This could be nullptr too. |
include/memory | ||
---|---|---|
1259 | This pointed out a number of things that could be fixed, so did that and updated. |
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.
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.
I'm fine with this given what the author said about nullptr already being used in those headers.
Did you try to build libc++ or run the tests before submitting this?
include/__threading_support | ||
---|---|---|
323 | This one is wrong. |
In <algorithm>, you missed a couple of (value_type*)0. Line 3356, 3369, 3486 and 3499.
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> |
include/__threading_support | ||
---|---|---|
323 | __libcpp_thread_t is an alias for an operating-system specific type. You can't compare it to nullptr. |
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.
libcxx/include/memory | ||
---|---|---|
4371 ↗ | (On Diff #307402) | Unneeded chang for use_count(). It's a long. |
This one is wrong.