This is an archive of the discontinued LLVM Phabricator instance.

[libc++][P0174] Deprecated/removed parts of default allocator.
ClosedPublic

Authored by mpark on Nov 12 2019, 3:40 AM.

Event Timeline

mpark created this revision.Nov 12 2019, 3:40 AM
mpark updated this revision to Diff 228853.Nov 12 2019, 3:59 AM

Updated to use allocator_traits<>::max_size().

mpark updated this revision to Diff 228855.Nov 12 2019, 4:03 AM

Fixed the static_cast result out of allocator<const T>.

Harbormaster completed remote builds in B40801: Diff 228855.
mpark updated this revision to Diff 228858.Nov 12 2019, 4:20 AM

Added a new test allocate.depr_in_cxx17.fail.cpp.

mpark updated this revision to Diff 228864.Nov 12 2019, 4:32 AM

Fixed the allocate.depr_in_cxx17.fail.cpp test.

mpark updated this revision to Diff 228975.EditedNov 12 2019, 3:31 PM
  • Replaced use of allocator_traits::max_size with implementation of max_size(). This will be replaced to allocator_traits::max_size once allocator::max_size and allocator_traits::max_size are both marked constexpr.
  • Removed deprecated attributes from C++03 codepaths.
  • Moved/added more tests.
mpark updated this revision to Diff 229007.Nov 12 2019, 10:24 PM

Refined the tests.

ldionne requested changes to this revision.Nov 13 2019, 8:57 AM
ldionne added inline comments.
libcxx/include/memory
118–125

Woooh, thanks for the cleanup.

1361

What! I didn't know you could surround literally any piece of code with these pragmas! It does make sense though, since it's a preprocessor thing. Clang will really take the pragma into account when parsing just that part of the declaration?

1394

If the return type of __a.construct(...) somehow hijacks operator,, this won't work. But I don't care, don't change it.

An Allocator's construct method is supposed to return void -- if someone's that clever, they deserve to be taught better.

1871

I'm OK with this TODO since I saw you fixed it in a subsequent patch.

libcxx/test/libcxx/depr/depr.default.allocator/allocator.members/address.depr_in_cxx17.fail.cpp
15–16

Wait, you shouldn't get rid of this test pre-C++17. You should mark it as REQUIRES: c++03, ..., c++14. Or did I miss something and you did not actually remove this test?

This revision now requires changes to proceed.Nov 13 2019, 8:57 AM
mpark marked 8 inline comments as done.Feb 13 2020, 5:23 AM
mpark added inline comments.
libcxx/include/memory
1361

I believe so!

1394

+1

libcxx/test/libcxx/depr/depr.default.allocator/allocator.members/address.depr_in_cxx17.fail.cpp
15–16

This test has been moved to libcxx/test/libcxx/depr/depr.default.allocator/allocator.members/address.cxx2a.pass.cpp, following the pattern for auto_ptr which have auto_ptr.cxx1z.pass.cpp and auto_ptr.depr_in_cxx11.fail.cpp.

mpark marked 4 inline comments as done.Feb 13 2020, 5:25 AM
mpark added inline comments.
libcxx/include/memory
1361

Never mind, You were right. I can't do this 😅

mpark updated this revision to Diff 244577.Feb 14 2020, 12:35 AM

Fixed build error.

ldionne accepted this revision.Mar 4 2020, 8:58 AM
ldionne added inline comments.
libcxx/test/libcxx/depr/depr.default.allocator/allocator_types.depr_in_cxx17.fail.cpp
35

I'll admit I'm not the biggest fan of those escape hatches. Let's leave it here to avoid a discussion, but in the future we may want to have a discussion about whether we want to be less forgiving about those.

This revision is now accepted and ready to land.Mar 4 2020, 8:58 AM
This revision was automatically updated to reflect the committed changes.
ldionne added inline comments.Mar 4 2020, 1:08 PM
libcxx/test/libcxx/depr/depr.default.allocator/allocator.members/allocate.depr_in_cxx17.fail.cpp
17 ↗(On Diff #248217)

Just as a statement of how tricky it is to get things right the first time in libc++ (cause we're talking about it elsewhere), this broke the GCC-trunk C++17 bot on Ubuntu, and only that one. Finding it required firing my docker image with a similar configuration to the bot and running another test that we'd expect to fail, but was instead skipped. That hinted me that this test should be unsupported too, and I found the typo.

dblaikie added inline comments.
libcxx/include/memory
717
Herald added a reviewer: Restricted Project. · View Herald TranscriptMay 11 2021, 9:11 AM
ldionne added inline comments.
libcxx/include/memory
717

Thanks for the heads up, indeed we need to fix this. I'm doing that now.

Thanks @jwakely for the report BTW.