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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik marked 2 inline comments as done.
philnik added inline comments.


================
Comment at: libcxx/include/__memory/allocator.h:112
+    allocation_result<_Tp*> allocate_at_least(size_t __n) {
+        return {allocate(__n), __n};
+    }
----------------
var-const wrote:
> 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?
I don't think there is anything preventing an implementation from over-allocating, but I don't know of a standard C API for over-allocating. We can of course special-case this for malloc implementations that have an API for over-allocating.


================
Comment at: libcxx/include/string:3293
+                __new_data = __allocation.ptr;
+                __target_capacity = __allocation.count - 1;
         #ifndef _LIBCPP_NO_EXCEPTIONS
----------------
var-const wrote:
> 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?
Feel free to review D119628! But maybe you should wait until I land D110598.


================
Comment at: libcxx/include/vector:2346
+        __size_ = 0;
+        __cap() = __allocation.count;
+    }
----------------
var-const wrote:
> 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?
AFAICT the number of `size_type`s is stored, so no, this doesn't require translation. The allocator returns the number of allocated objects, so the same number that is saved internally.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122877