[libcxx-commits] [PATCH] D102727: Use precise growth rather than amortized growth in std::string::resize/__resize_default_init.

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

Quuxplusone added inline comments.


================
Comment at: libcxx/include/string:3280
+    if (__n > __sz) {
+        if (__n > capacity()) __shrink_or_extend(__recommend(__n));
         append(__n - __sz, __c);
----------------
mvels wrote:
> ldionne wrote:
> > Please put the `__shrink_or_extend` call on its own line (same below).
> as per my general comment, I'd suggest putting this inside #if defined(_LIBCPP_ABI_UNSTABLE) 
> But this change has nothing to do with the ABI.

It has to do with ABI in the sense that one can imagine compiling one TU with this patch, and one TU without this patch, and then linking them together, and having the finished program misbehave; whereas if both TUs were compiled with the old library, they'd have worked OK. In that sense, anything having to do with behavior — and that is not detectable in "API" — is classified as "ABI" by definition.

Note that this should absolutely not be guarded by `_LIBCPP_ABI_UNSTABLE`; that would make it a "receding target" that would never become the default, not even when libc++ moved to "ABI version 2," because when the stable version is 2, the unstable version will become 3. If we were going to do this as an ABI flag, we'd:

- Add a new flag `_LIBCPP_ABI_EXACT_VECTOR_RESIZE` circa https://github.com/llvm/llvm-project/blob/2ff5a56/libcxx/include/__config#L95-L96
- Guard the new lines in `<vector>` under `_LIBCPP_ABI_EXACT_VECTOR_RESIZE`
- (We don't mention `_LIBCPP_ABI_VERSION>=2` in the `<vector>` header, because (A) that's too easy to thinko e.g. as `_LIBCPP_ABI_VERSION==2`, and (B) it wouldn't be self-documenting. Every libc++ ABI change comes with a greppable name, so we don't end up with a confusing web of "miscellaneous ABI version 2 changes" that are hard to disentangle. This also serves as a good brake on people adding "miscellaneous version-2 changes" willy-nilly.)

Then users who compile libc++ with ABI version 2 (or newer) will receive the new behavior.

FWIW, personally I don't care whether this change is done under an ABI flag, or unconditionally, or not-at-all.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102727