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

LGTM at this point mod nits.
However, I still express no opinion as to whether we want to do this at all.
I'm just saying that //if// we wanted to do this, //then// I think this patch now reflects the appropriate way to do it.



================
Comment at: libcxx/test/libcxx/strings/basic.string/string.capacity/resize.exact.cpp:28
+  const size_t kSsoSize = S().capacity();
+  const size_t kAlign = 16;
+  for (int i = kSsoSize + 1; i < 1 << 10; ++i) {
----------------
Should this be `alignof(std::max_align_t)`? Or what's the significance of `16` — how was it computed?


================
Comment at: libcxx/test/libcxx/strings/basic.string/string.capacity/resize.exact.cpp:29
+  const size_t kAlign = 16;
+  for (int i = kSsoSize + 1; i < 1 << 10; ++i) {
+    S s;
----------------
Style nit: I'd prefer to see `i < (1 << 10)` or simply `i < 1024`, instead of the unparenthesized `i < 1 << 10`. Mixing `<` and `<<` reads too much like a typo.


================
Comment at: libcxx/test/libcxx/strings/basic.string/string.capacity/resize.exact.cpp:50
+    typedef std::string S;
+    test<S>();
+  }
----------------
Please also test `std::wstring`, for completeness. (And as "documentation" that this optimization is intended to apply to all `basic_string` specializations, not just to `std::string` specifically.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102727