[libcxx-commits] [PATCH] D101752: Speedup to_string for integers using zero-copy.

[Mark de Wever via Phabricator via libcxx-commits]

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

Thanks for your patch.
Please investigate the ARM build errors.



================
Comment at: libcxx/src/string.cpp:429
+    std::string buf;
+    buf.resize(buf.capacity());
+    const auto res = std::to_chars(buf.data(), buf.data() + buf.size(), v);
----------------
Can you add an assert the buffer-size is large enough? You now depend on an implementation detail of `std::string`. Something like:
_LIBCPP_ASSERT(buf.size(), std::numeric_limits<V>::digits10 + 2, "Capacity is too small");
If somebody adds 128-bit support this will probably break.



================
Comment at: libcxx/src/string.cpp:463
+wstring to_wstring(unsigned long val)      { return i_to_wstring(val); }
+wstring to_wstring(unsigned long long val) { return i_to_wstring(val); }
 
----------------
Roman-Koshelev wrote:
> Quuxplusone wrote:
> > Do you have a benchmark showing the performance impact? I think you //should// try to quantify the impact somehow — after all, all you're saving is a very predictable branch and a single stack-to-stack memcpy, right? (OTOH, I don't see how this could possibly //hurt// performance.)
> No need to make copying where it is not needed. 
> 
> Also 
> ```
> std::string buf;
> buf.resize(buf.capacity());
> ```
> with c++20 completely constexpr
+1 for a benchmark. The change that might degrade the performance are the resize calls.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101752