Details
- Reviewers
ldionne Mordante var-const huixie90 - Group Reviewers
Restricted Project - Commits
- rG181cce6b696e: [libc++] Implement P0339R6 (polymorphic_allocator<> as a vocabulary type)
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
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>. |
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?
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.
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. |
libcxx/test/std/utilities/utility/mem.res/mem.poly.allocator.class/mem.poly.allocator.mem/default_type.compile.pass.cpp | ||
---|---|---|
1 | 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. |