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

[Nikolas Klauser via Phabricator via libcxx-commits]

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:
> 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.
I think you are right that the implementation is allowed to return more memory than was requested. The problem is that we have no way to tell if the underlying allocator ever over-allocates. We don't have an API that can tell us that. There is no C-version of `allocate_at_least`. The only way to support this currently is through extensions. But I don't think we should add a TODO there. This TODO would never be removed, and what's the point of a TODO then? We also don't have a `// TODO: optimize sort`.


================
Comment at: libcxx/test/std/utilities/memory/allocator.traits/allocate_at_least.pass.cpp:30
+
+template <class T>
+class has_allocate_at_least {
----------------
var-const wrote:
> Nit: since these helpers are always instantiated with `int`, they probably don't need to be templates at all.
I think you have to be able to rebind an allocator, so making this a non-template class would make this technically not an allocator.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122877