[PATCH] D41458: [libc++][C++17] Elementary string conversions for integral types

[Zhihao Yuan via Phabricator via cfe-commits]

lichray marked 7 inline comments as done.
lichray added a comment.
Herald added a subscriber: christof.

Pending patch update due to poor network.



================
Comment at: include/charconv:90
+
+enum class _LIBCPP_ENUM_VIS chars_format
+{
----------------
EricWF wrote:
> enum types should have their underlying integer type explicitly specified. Otherwise their size and ABI can break when users specify `-short-enums`
The standard requires `enum class` to default to `int`; IIUC `enum class` is not affected by `-fshort-enums`.


================
Comment at: include/support/itoa/itoa.h:1
+// -*- C++ -*-
+//===--------------------- support/itoa/itoa.h ----------------------------===//
----------------
EricWF wrote:
> I would rather not introduce another header for this. I think it should go directly in the `charconv` header.
We may have separated floating point headers coming as well...


================
Comment at: include/support/itoa/itoa.h:47
+
+static constexpr uint32_t __pow10_32[] = {
+    UINT32_C(0),          UINT32_C(10),       UINT32_C(100),
----------------
EricWF wrote:
> I suspect we can use `__pow10_64` in the 32 bit case as well to avoid emitting another static global.
I have functions returning them as array references.


================
Comment at: include/support/itoa/itoa.h:54
+
+#if __has_builtin(__builtin_clzll)
+
----------------
EricWF wrote:
> Use `__clz` from `<algorithm>`. You can assume we always have that.
Factor `__clz` out of `algorithm` may be too much for this review.  I would like to see a subsequent patch to take care of all the related portability fixes.


Repository:
  rCXX libc++

https://reviews.llvm.org/D41458