Page MenuHomePhabricator

[libc++] Implement P0339R6 (polymorphic_allocator<> as a vocabulary type)
ClosedPublic

Authored by philnik on Nov 9 2022, 1:43 PM.

Diff Detail

Event Timeline

philnik created this revision.Nov 9 2022, 1:43 PM
philnik requested review of this revision.Nov 9 2022, 1:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 9 2022, 1:43 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne requested changes to this revision.Nov 21 2022, 8:50 AM
ldionne added inline comments.
libcxx/test/std/utilities/utility/mem.res/mem.poly.allocator.class/mem.poly.allocator.mem/allocate_vocabulary.pass.cpp
1 ↗(On Diff #474348)

You're going to hate this, but we have one test file per public API function. Let's be consistent.

51 ↗(On Diff #474348)

Not attached to line: Please add an explicit test that polymorphic_allocator<> is the same as polymorphic_allocator<byte>.

56 ↗(On Diff #474348)

Typo

58 ↗(On Diff #474348)

You're only testing with std::pmr::polymorphic_allocator<>, but in reality this paper added new methods for all specializations of polymorphic_allocator. For example, I'd expect to have a test that we can call allocate_bytes and deallocate_bytes on a polymorphic_allocator<Foo>, and even allocate_object<int>() on a polymorphic_allocator<Foo>.

This revision now requires changes to proceed.Nov 21 2022, 8:50 AM
LRFLEW added a subscriber: LRFLEW.Nov 28 2022, 10:29 PM

The implementation of polymorphic_allocator::allocate_object here doesn't follow the changes in LWG 3237 and LWG 3310. SIZE_MAX should be replaced with numeric_limits<size_t>::max() (perhaps using a templated version of __max_size()), and the error condition should throw bad_array_new_length instead of length_error.

Also, on the topic of LWG 3304, it looks like polymorphic_allocator::allocate is supposed to be marked as [[nodiscard]] in C++20. Would it make sense for this patch to also include that change as well?

Lastly, I noticed that the existing code is using #if _LIBCPP_STD_VER > 14, but the code added here is using #if _LIBCPP_STD_VER >= 20. Would it make more sense to use #if _LIBCPP_STD_VER > 17 instead?

The implementation of polymorphic_allocator::allocate_object here doesn't follow the changes in LWG 3237 and LWG 3310. SIZE_MAX should be replaced with numeric_limits<size_t>::max() (perhaps using a templated version of __max_size()), and the error condition should throw bad_array_new_length instead of length_error.

Thanks for the info! I'll apply them in this patch, since they are almost trivial. It looks like we accidentally marked LWG3237 as done already.

Also, on the topic of LWG 3304, it looks like polymorphic_allocator::allocate is supposed to be marked as [[nodiscard]] in C++20. Would it make sense for this patch to also include that change as well?

See D137597 for that.

Lastly, I noticed that the existing code is using #if _LIBCPP_STD_VER > 14, but the code added here is using #if _LIBCPP_STD_VER >= 20. Would it make more sense to use #if _LIBCPP_STD_VER > 17 instead?

We decided to use _LIBCPP_STD_VER >= x for new code.

philnik updated this revision to Diff 478555.Nov 29 2022, 6:16 AM
philnik marked 4 inline comments as done.

Address comments

ldionne accepted this revision.Dec 1 2022, 9:09 AM

LGTM with green CI and comments applied. Thanks @LRFLEW for the comments, those were good observations.

libcxx/include/__memory_resource/polymorphic_allocator.h
113
libcxx/test/std/utilities/utility/mem.res/mem.poly.allocator.class/mem.poly.allocator.mem/allocate_bytes.pass.cpp
1 ↗(On Diff #478555)

Let's name this allocate_deallocate_bytes.pass.cpp. Same for allocate_object.pass.cpp.

61 ↗(On Diff #478555)

return 0;

Everywhere.

This revision is now accepted and ready to land.Dec 1 2022, 9:09 AM
philnik updated this revision to Diff 479430.Dec 1 2022, 2:23 PM
philnik marked 3 inline comments as done.
  • Rebased
  • Address comments
philnik updated this revision to Diff 479565.Dec 2 2022, 2:31 AM

Try to fix CI

philnik updated this revision to Diff 479577.Dec 2 2022, 3:06 AM

Try to fix CI

philnik updated this revision to Diff 479586.Dec 2 2022, 3:45 AM

Next try

philnik updated this revision to Diff 479588.Dec 2 2022, 3:55 AM

Next try

ldionne added inline comments.Dec 2 2022, 7:50 AM
libcxx/test/std/utilities/utility/mem.res/mem.poly.allocator.class/mem.poly.allocator.mem/default_type.compile.pass.cpp
2

This is XPASSing on macos backdeployment because this would only fail at runtime. Mark this one as UNSUPPORTED on the backdeployment targets instead of XFAIL.

philnik updated this revision to Diff 479920.Dec 4 2022, 6:44 AM
philnik marked an inline comment as done.

Try to fix CI

philnik updated this revision to Diff 480529.Dec 6 2022, 9:34 AM

Try to fix CI

philnik updated this revision to Diff 480681.Dec 6 2022, 4:28 PM

Next try

philnik updated this revision to Diff 480707.Dec 6 2022, 5:06 PM

Next try

This revision was landed with ongoing or failed builds.Dec 7 2022, 9:50 AM
This revision was automatically updated to reflect the committed changes.