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

[Zhihao Yuan via Phabricator via cfe-commits]

lichray added a comment.

I will pick up the changes later next week.



================
Comment at: include/charconv:89
+_LIBCPP_BEGIN_NAMESPACE_STD
+
+enum class _LIBCPP_ENUM_VIS chars_format
----------------
EricWF wrote:
> We need to hide these names when `_LIBCPP_STD_VER < 17`, since we're not allowed to introduce new names into namespace `std` in older dialects.
But this header is backported to C++11, so I intended to not to guard it.


================
Comment at: include/charconv:122
+template <typename _Tp>
+inline _LIBCPP_INLINE_VISIBILITY auto
+__to_unsigned(_Tp __x) -> typename make_unsigned<_Tp>::type
----------------
EricWF wrote:
> Same as above. There's no reason to deviate from the typical libc++ style and use trailing return types here.
libc++ doesn't have one uniformed style.  For some reason I found this style formats better with clang-format.


================
Comment at: include/charconv:151
+
+#if __has_builtin(__builtin_clzll)
+    if (__tx::digits <= __diff || __tx::width(__value) <= __diff)
----------------
EricWF wrote:
> `<algorithm>` already has a `__clz` wrapper for `__builtin_clz` et al. We should use that instead. That also allows us to get rid of the fallback implementation, and it correctly uses the builtin for compilers like GCC which don't provide `__has_builtin`.
I saw that, and I agree this can be improved, however `<algorithm>` would be too heavy to include here.  Thoughts?


================
Comment at: include/support/itoa/itoa.h:81
+#if __has_builtin(__builtin_clzll)
+    static _LIBCPP_INLINE_VISIBILITY auto width(_Tp __v) -> int
+    {
----------------
EricWF wrote:
> `width`, `convert` and `pow` need to be reserved names.
These names are standard names (functions), so users can't provide function style macros to override them.  Am I wrong on that?


================
Comment at: include/support/itoa/itoa.h:123
+inline _LIBCPP_INLINE_VISIBILITY bool
+__mul_overflowed(unsigned char __a, _Tp __b, unsigned char& __r)
+{
----------------
EricWF wrote:
> `__builtin_mul_overflow` works for `char` and `short`, so can't we just call that?
I don't think so, it "works" after promotion.


================
Comment at: include/support/itoa/itoa.h:161
+template <typename _Tp>
+struct _LIBCPP_HIDDEN traits : __traits<_Tp>
+{
----------------
EricWF wrote:
> `traits` needs to be a reserved name, and preferably a more descriptive one.
Similarly It's a standard name (std::string typedef), so user can't use a non-function style macro name to override it, IIUC.


================
Comment at: include/support/itoa/itoa.h:188
+
+    template <typename _It1, typename _It2, class _Up>
+    static _LIBCPP_INLINE_VISIBILITY auto
----------------
EricWF wrote:
> What's wrong with using `std::inner_product`?
`__first != __last1` instead of `<`


================
Comment at: src/support/itoa/itoa.cpp:1
+// -*- C++ -*-
+//===----------------------------------------------------------------------===//
----------------
EricWF wrote:
> This file should be renamed `charconv.cpp` and moved into the main source directory.
We are going to have floating point cpp files so I don't think that one charconv.cpp is enough.


================
Comment at: src/support/itoa/itoa.cpp:35
+
+#define APPEND1(i)                              \
+    do                                          \
----------------
EricWF wrote:
> Any reason these can't be `static` functions? The compiler should optimize them away nicely.
Although yes, but that's what the author provides.  It's an implementation file, so it doesn't matter I guess.


Repository:
  rCXX libc++

https://reviews.llvm.org/D41458