[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 added a comment.

@philnik explained what I was misunderstanding on Discord. I believe this patch is correct, and the code might be easier to understand if we clarify a few things in the code, like I suggest below.

Basically, what I understand is that when Howard initially wrote string, he made `__recommend()` exclude the null terminator so that it is consistent with `capacity()`. However, the capacity stored in the string object always included the null-terminator, which led to having to adjust the result of `__recommend()` in a bunch of places. This patch moves the inconsistency: it makes `__recommend()` and the stored capacity consistent, at the cost of making `__recommend()` and `capacity()` inconsistent with each other, needing a couple of adjustments (two to be exact, where I requested some comments). Given this, I don't think this patch is an absolute must-do -- we're just shifting complexity around. However, I do feel like it does reduce the complexity overall, so I'm favourable.



================
Comment at: libcxx/include/string:934-936
     _LIBCPP_INLINE_VISIBILITY size_type capacity() const _NOEXCEPT
         {return (__is_long() ? __get_long_cap()
                              : static_cast<size_type>(__min_cap)) - 1;}
----------------



================
Comment at: libcxx/include/string:1495
     _LIBCPP_INLINE_VISIBILITY
     void __set_long_cap(size_type __s) _NOEXCEPT
         {__r_.first().__l.__cap_  = __long_mask | __s;}
----------------
We should add a comment here (I would have added it to the actual data member, but it's hard because it's defined in the weirdest way) saying something like this:

```
// Note that the capacity we store always includes the null-terminator.
```


================
Comment at: libcxx/include/string:1536-1537
     enum {__alignment = 16};
     static _LIBCPP_INLINE_VISIBILITY
     size_type __recommend(size_type __s) _NOEXCEPT
+    {
----------------
Can you please add a comment explaining what this returns? I.e. something like

```
// Returns the recommended capacity to store a string of size __s, including the null-terminator.
// Note that unlike __recommend(), string::capacity() returns the capacity *excluding* the null-terminator.
```


================
Comment at: libcxx/include/string:3240
     __target_capacity = __recommend(__target_capacity);
-    if (__target_capacity == capacity()) return;
+    if (__target_capacity == capacity() + 1) return;
 
----------------
WDYT?


================
Comment at: libcxx/include/string:3251
     size_type __target_capacity = __recommend(size());
-    if (__target_capacity == capacity()) return;
+    if (__target_capacity == capacity() + 1) return;
 
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119628