[libcxx-commits] [PATCH] D137739: [libc++] Implement P0339R6 (polymorphic_allocator<> as a vocabulary type)

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/test/std/utilities/utility/mem.res/mem.poly.allocator.class/mem.poly.allocator.mem/allocate_vocabulary.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
You're going to hate this, but we have one test file per public API function. Let's be consistent.


================
Comment at: libcxx/test/std/utilities/utility/mem.res/mem.poly.allocator.class/mem.poly.allocator.mem/allocate_vocabulary.pass.cpp:51
+  bool do_is_equal(const memory_resource& ptr) const noexcept override { return &ptr == this; }
+};
+
----------------
Not attached to line: Please add an explicit test that `polymorphic_allocator<>` is the same as `polymorphic_allocator<byte>`.


================
Comment at: libcxx/test/std/utilities/utility/mem.res/mem.poly.allocator.class/mem.poly.allocator.mem/allocate_vocabulary.pass.cpp:56
+  size_t last_alginment = 0;
+  TrackingMemRes resource(&last_size, &last_alginment);
+
----------------
Typo


================
Comment at: libcxx/test/std/utilities/utility/mem.res/mem.poly.allocator.class/mem.poly.allocator.mem/allocate_vocabulary.pass.cpp:58
+
+  std::pmr::polymorphic_allocator<> allocator(&resource);
+
----------------
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>`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137739/new/

https://reviews.llvm.org/D137739