This is an archive of the discontinued LLVM Phabricator instance.

[libc++abi] Use std::nullptr_t instead of declaring it manually
ClosedPublic

Authored by rprichard on Oct 31 2022, 3:44 PM.

Details

Summary

Sometimes libc++'s stddef.h wrapper gets included, which defines
::nullptr_t. This test is compiled with -Wshadow -Werror, so shadowing
::nullptr_t with a nullptr_t in main is an error. Include cstddef,
which is guaranteed to define std::nullptr_t in C++11 and forward.

Diff Detail

Event Timeline

rprichard created this revision.Oct 31 2022, 3:44 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 31 2022, 3:44 PM
rprichard requested review of this revision.Oct 31 2022, 3:44 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 31 2022, 3:44 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne accepted this revision.Nov 1 2022, 9:22 AM

LGTM with drive-by fix.

libcxxabi/test/catch_reference_nullptr.pass.cpp
9

While we're at it.

This revision is now accepted and ready to land.Nov 1 2022, 9:22 AM
rprichard updated this revision to Diff 472443.Nov 1 2022, 4:28 PM

Remove extraneous comma.

EricWF added a subscriber: EricWF.Nov 1 2022, 4:30 PM

I don't think this helps, but I don't think it hurts. The correctness of the tests requires std::nullptr_t to be decltype(nullptr), so /shrug

I don't think this helps, but I don't think it hurts. The correctness of the tests requires std::nullptr_t to be decltype(nullptr), so /shrug

The question is whether the <cstdlib> header is allowed to define a ::nullptr_t. If it does define it (it does with the Android NDK), then the test doesn't compile because -Wshadow complains about the local nullptr_t. (I suspect the issue is that bionic's stdlib.h includes malloc.h, which includes stddef.h without first declaring any __need_* macros. When libc++'s stddef.h is included, it defines ::nullptr_t.) It could be an issue with the bionic headers, but that's also probably hard to clean up without breaking stuff?

EricWF added a comment.Nov 1 2022, 5:38 PM

I don't think this helps, but I don't think it hurts. The correctness of the tests requires std::nullptr_t to be decltype(nullptr), so /shrug

The question is whether the <cstdlib> header is allowed to define a ::nullptr_t. If it does define it (it does with the Android NDK), then the test doesn't compile because -Wshadow complains about the local nullptr_t. (I suspect the issue is that bionic's stdlib.h includes malloc.h, which includes stddef.h without first declaring any __need_* macros. When libc++'s stddef.h is included, it defines ::nullptr_t.) It could be an issue with the bionic headers, but that's also probably hard to clean up without breaking stuff?

I think this change is fine to proceed. Maybe for the super paranoid of us (me), we could static_assert(is_same<std::nullptr_t, decltype(nullptr)>).

That said, you're also free to ignore everything I just said. This test would probably fail if the types weren't the same, and it'll be a very weird day if the bug I'm trying to defend against actually comes to pass.

rprichard updated this revision to Diff 472778.Nov 2 2022, 2:52 PM

Add static_assert

rprichard updated this revision to Diff 473339.Nov 4 2022, 2:19 PM

Avoid std::is_same_v, which was added in C++17, in favor of std::is_same::value.