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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added inline comments.


================
Comment at: libcxx/include/__memory/allocator.h:112
+    allocation_result<_Tp*> allocate_at_least(size_t __n) {
+        return {allocate(__n), __n};
+    }
----------------
philnik wrote:
> 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.
Maybe I'm misreading the paper (I wasn't aware of it before seeing this patch), but it seemed like the whole purpose of `allocate_at_least` is to avoid wasting capacity in case an allocator overallocates. I'm not clear, however, if this only applies to user-provided allocators, or if it's meant specifically for `std::allocator`, but given the wording changes for `std::allocator`, I presume the latter.

I don't know if the underlying allocator machinery we're currently using is overallocating and if there's a way to query the actual allocated size. However, I think that:
- if our underlying allocations never overallocate, we should add a comment to that effect to `std::allocator<T>::allocate_at_least`;
- otherwise, we should add a TODO.

In short, if I'm reading the paper correctly, it's meant to bring a behavioral change, whereas with the current implementation in this patch, the behavior will be exactly equivalent. Once again, I might well be missing or misreading something -- please point it out if I do.


================
Comment at: libcxx/include/string:3293
+                __new_data = __allocation.ptr;
+                __target_capacity = __allocation.count - 1;
         #ifndef _LIBCPP_NO_EXCEPTIONS
----------------
philnik wrote:
> 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.
Interesting, thanks for the link. I didn't realize there's a much bigger discussion hidden here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122877

STAMPS
actor(@var-const) application(Differential) author(@philnik) herald(H157) herald(H208) herald(H343) herald(H426) herald(H576) herald(H864) monogram(D122877) object-type(DREV) phid(PHID-DREV-gktvhbsp76wymefvqkvo) reviewer(#libc) reviewer(@ldionne) reviewer(@Mordante) reviewer(@var-const) revision-repository(rG) revision-status(needs-revision) subscriber(@arichardson) subscriber(@libcxx-commits) subscriber(@mgorny) tag(#all) tag(#libc) via(web)