[libcxx-commits] [PATCH] D119628: [libc++] Don't return alloc_size - 1 in __recommend to add 1 when allocating

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/include/string:2261-2264
     size_type __cap = __old_cap < __ms / 2 - __alignment ?
                           __recommend(_VSTD::max(__old_cap + __delta_cap, 2 * __old_cap)) :
                           __ms - 1;
+    pointer __p = __alloc_traits::allocate(__alloc(), __cap);
----------------
Quuxplusone wrote:
> (1) I'm still very unsure that this is correct. @philnik, can you maybe make up a cheat-sheet that shows which of these numbers will include +1 for the null terminator and which won't?
> - `x = capacity()`
> - `__set_long_cap(x)`
> - `x = __recommend(...)`
> - `__grow_by(x, ...)`
> - `__grow_by_and_replace(x, ...)`
> 
> Ideally, in the end, they'd all use the same meaning for "capacity" — which means it should //not// include the null terminator, because `capacity()` doesn't include the null terminator.
> 
> (2) On this diff in particular, aren't you accidentally failing to +1 the capacity when `(__old_cap < __ms / 2 - __alignment)`? Please investigate **and add a regression test for what you find.**
(1)
| function | `x` includes null-terminator? |
| `x = capacity()` | N |
| `__set_long_cap(x)` | Y |
| `x = __recommend()` | Y |
| `__grow_by(x, ...)` | N |
| `__grow_by_and_replace(x, ...)` | N |
| `x = __get_long_cap()` | Y |

It's not possible for all of these to have the same meaning, because of ABI. We can't change `__grow_by{, _and_replace}` and `__{get, set}_long_cap()` because of ABI and `capacity()` because the standard demands different behavior.

(2) Do you mean the false branch? In that case I //think// it should be `__ms`. I'll add a regression test.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119628