[libcxx-commits] [libcxx] [libc++][test] Fix increasing_allocator to meet `Cpp17Allocator` requirements (PR #115671)

[Nikolas Klauser via libcxx-commits]

philnik777 wrote:

> > The intention of `allocate_at_least` is that the allocator tells us how much it overallocated due to its allocation algorithm, not that it actually allocates more. If you implement the interface properly `allocate` will only hide the overallocation, not prevent it.
> 
> I agree, and I don't see how this makes the allocator more conforming than it used to be. However, I do think the code makes more sense after the patch than it did before the patch, because implementing `allocate()` using `std::allocator::allocate` makes more sense than doing so with `allocate_at_least`.

I'm not disagreeing here. The code is an improvement, but I don't think the commit message makes sense.

> > Actually, the tests failed with MSVC STL (or MSVC, the compiler). MSVC detects size mismatch of deallocation in constant evaluation. I guess we can't reliably perform over allocation in constant evaluation.
> 
> ```c++
> constexpr std::allocation_result<T*> allocate_at_least(std::size_t n);
> constexpr T* allocate(std::size_t n) {
>   auto res = allocate_at_least(n);
>   return res.ptr;
> }
> constexpr void deallocate(T* p, std::size_t n) noexcept {
>   // n may be wrong: it must match res.count which may not be the case if we used allocate_at_least above
>   std::allocator<T>{}.deallocate(p, n);
> }
> ```
> 
> Is that what you're saying? I would understand why the current code is a problem in that case, however http://eel.is/c++draft/allocator.members#10.1 says:
> 
> > If p is memory that was obtained by a call to allocate_at_least, let ret be the value returned and req be the value passed as the first argument to that call[.](http://eel.is/c++draft/allocator.members#10.1.sentence-1) p is equal to ret.ptr and n is a value such that req  ≤ n  ≤ ret[.](http://eel.is/c++draft/allocator.members#10.1.sentence-2)count.
> 
> In other words, I think it's supposed to be just fine to call `std::allocator::deallocate` with a `n` that is less than the count returned by a previous `allocate_at_least`.

The calling code is fine. The mismatch between the `std::allocator::allocate()` and `std::allocator::deallocate()` is the problem. Note how `increasing_allocator::allocate_at_least` calls `std::allocator::allocate(n + 1000)` in some cases, but `increasing_allocator::deallocate()` always just calls `std::allocator::deallocate(n)`.

Actually, I just noticed that the current code is still wrong, since there is still potentially the same mismatch when calling `increasing_allocator::allocate_at_least` and `increasing_allocator::deallocate`.


https://github.com/llvm/llvm-project/pull/115671