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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
Comment at: libcxx/include/string:1541
+            return static_cast<size_type>(__min_cap);
+        size_type __guess = __align_it<sizeof(value_type) <__alignment ? __alignment / sizeof(value_type) : 1> (__s + 1);
+        if (__guess == __min_cap + 1)
----------------
Clang-format strikes again. (If you must clang-format, please remember to sanity-check the code when it's finished.)


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119628