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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

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.



================
Comment at: libcxx/include/string:3240-3242
+    if (__target_capacity == capacity() + 1) return;
 
+    __shrink_or_extend(__target_capacity - 1);
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119628