[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 a comment.
Herald added a project: All.

In D119628#3348916 <https://reviews.llvm.org/D119628#3348916>, @ldionne wrote:

> I think we need to understand the situation with `reserve` and `shrink_to_fit` before moving forward with this, otherwise I think we might just be making the code more difficult to understand.
>
> Also, this is pretty tricky because it's possible that already compiled code has the old definition of e.g. `__init` inlined, and hence the capacity of the string is set to `N + 1`. If such a string is passed to code compiled with the new definition, the new code would think that the capacity represents `N`, not `N + 1`, and would most likely mis-behave. In other words, I think this is ABI breaking.

I'm not changing what is saved in long cap. That has always been the capacity including the null terminator AFAICT. Or is there another potential ABI break that I'm not seeing?



================
Comment at: libcxx/include/string:3240-3242
+    if (__target_capacity == capacity() + 1) return;
 
+    __shrink_or_extend(__target_capacity - 1);
----------------
ldionne wrote:
> Quuxplusone wrote:
> > This diff strikes me as questionable (which probably means the entire patch is questionable). The logic here previously was, "If the target capacity is equal to our current capacity, nothing to do, return. Otherwise, shrink or extend to the target capacity." //Now// the logic is, "If the target capacity is one greater than our current capacity, do nothing! Otherwise, shrink or extend to one //less// than the target capacity." Regardless of whether the arithmetic works out in practice, that logic seems very wrong. Maybe it can be saved by renaming some variables, e.g. maybe `__target_capacity` doesn't actually represent a capacity? Or maybe prior to line 3239 it's a capacity but after line 3239 it's a one-greater-than-a-capacity?
> I agree, this looks fishy to me. Are you certain this wasn't a bug previously and that it should instead be `if (__target_capacity == capacity()) return;` *even* in the new diff? Do we have a test for that?
IIUC `basic_string::capacity()` returns the number of characters without the null terminator while `__recommend()` (with this patch) returns the number of characters including the null terminator.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119628