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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
