[libcxx-commits] [PATCH] D122877: [libc++] Implement P0401R6 (allocate_at_least)

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const requested changes to this revision.
var-const added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/__memory/allocate_at_least.h:58
+  __ret.count = __n;
+  return __ret;
+}
----------------
Nit: can it be just `return {__alloc.allocate(__n), __n};` like above? I think braced-list initialization should be supported in old language modes.


================
Comment at: libcxx/include/__memory/allocate_at_least.h:64
+_LIBCPP_END_NAMESPACE_STD
+
+
----------------
Nit: remove one extra blank line (there currently are two blank lines next to each other).


================
Comment at: libcxx/include/__memory/allocator.h:16
 #include <__utility/forward.h>
-#include <cstddef>
 #include <new>
----------------
Nit: I think it would be easier to just follow "include-what-you-use". It's not obvious that `allocate_at_least.h` should guarantee to include `<cstddef>`.


================
Comment at: libcxx/include/__memory/allocator.h:112
+    allocation_result<_Tp*> allocate_at_least(size_t __n) {
+        return {allocate(__n), __n};
+    }
----------------
Hmm, if I'm reading this correctly, this is exactly equivalent to the old behavior -- even though we implement an interface that would allow an allocator to overallocate, the actual standard allocators never overallocate. Does this require a TODO? Or is this feature only for user-provided allocators?


================
Comment at: libcxx/include/string:3293
+                __new_data = __allocation.ptr;
+                __target_capacity = __allocation.count - 1;
         #ifndef _LIBCPP_NO_EXCEPTIONS
----------------
Question: this is to account for the terminating null, right? Would it be cleaner to remove the subtraction here and the `+ 1` addition below, instead of having to adjust the value twice?


================
Comment at: libcxx/include/vector:2346
+        __size_ = 0;
+        __cap() = __allocation.count;
+    }
----------------
Is this line equivalent to `this->__cap() = __n;` (where `__n = __external_cap_to_internal(__n);`)? In other words, does `__allocation.count` require the external->internal adjustment or not?


================
Comment at: libcxx/test/std/utilities/memory/allocator.traits/allocate_at_least.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
I think there also needs to be a test for `std::allocator<T>::allocate_at_least`. This test essentially checks that `std::allocate_at_least` is correctly forwarded to the allocator's `allocate_at_least`.


================
Comment at: libcxx/test/std/utilities/memory/allocator.traits/allocate_at_least.pass.cpp:21
+template <class T>
+class no_allocate_at_least {
+public:
----------------
Nit: make it a `struct` and avoid the `public:` label?


================
Comment at: libcxx/test/std/utilities/memory/allocator.traits/allocate_at_least.pass.cpp:30
+
+template <class T>
+class has_allocate_at_least {
----------------
Nit: since these helpers are always instantiated with `int`, they probably don't need to be templates at all.


================
Comment at: libcxx/test/std/utilities/memory/allocator.traits/allocate_at_least.pass.cpp:47
+    no_allocate_at_least<int> alloc;
+    std::same_as<std::allocation_result<int*>> auto ret = std::allocate_at_least(alloc, 1);
+    assert(ret.count == 1);
----------------
Nit: `decltype(auto)`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122877