[PATCH] D41458: [libc++][C++17] Elementary string conversions for integral types
Eric Fiselier via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Feb 9 20:31:47 PST 2018
EricWF added a comment.
Some initial thoughts.
@mclow.lists Are you planning on moving forward with your implementation as well?
================
Comment at: include/charconv:89
+_LIBCPP_BEGIN_NAMESPACE_STD
+
+enum class _LIBCPP_ENUM_VIS chars_format
----------------
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.
================
Comment at: include/charconv:90
+
+enum class _LIBCPP_ENUM_VIS chars_format
+{
----------------
enum types should have their underlying integer type explicitly specified. Otherwise their size and ABI can break when users specify `-short-enums`
================
Comment at: include/charconv:114
+template <typename _Tp>
+inline _LIBCPP_INLINE_VISIBILITY auto
+__complement(_Tp __x) -> _Tp
----------------
No reason for using trailing return type syntax here.
================
Comment at: include/charconv:122
+template <typename _Tp>
+inline _LIBCPP_INLINE_VISIBILITY auto
+__to_unsigned(_Tp __x) -> typename make_unsigned<_Tp>::type
----------------
Same as above. There's no reason to deviate from the typical libc++ style and use trailing return types here.
================
Comment at: include/charconv:151
+
+#if __has_builtin(__builtin_clzll)
+ if (__tx::digits <= __diff || __tx::width(__value) <= __diff)
----------------
`<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`.
================
Comment at: include/support/itoa/itoa.h:1
+// -*- C++ -*-
+//===--------------------- support/itoa/itoa.h ----------------------------===//
----------------
I would rather not introduce another header for this. I think it should go directly in the `charconv` header.
================
Comment at: include/support/itoa/itoa.h:24
+
+static constexpr uint64_t __pow10_64[] = {
+ UINT64_C(0),
----------------
I'm not sure I love having static globals in the headers, but I can't think of a better way to write this.
================
Comment at: include/support/itoa/itoa.h:29
+ UINT64_C(1000),
+ UINT64_C(10000),
+ UINT64_C(100000),
----------------
The `UINT64_C` and `UINT32_C` macros are non-standard, so I would rather not use them in a header.
Additionally, the compiler should correctly convert the integer type to the array's element type, no?
================
Comment at: include/support/itoa/itoa.h:47
+
+static constexpr uint32_t __pow10_32[] = {
+ UINT32_C(0), UINT32_C(10), UINT32_C(100),
----------------
I suspect we can use `__pow10_64` in the 32 bit case as well to avoid emitting another static global.
================
Comment at: include/support/itoa/itoa.h:54
+
+#if __has_builtin(__builtin_clzll)
+
----------------
Use `__clz` from `<algorithm>`. You can assume we always have that.
================
Comment at: include/support/itoa/itoa.h:57
+inline _LIBCPP_INLINE_VISIBILITY auto
+__u64digits10(uint64_t __x) -> int
+{
----------------
`__u64digits10` and `__u32digits10` can be folded into `width`, since it's the only caller.
================
Comment at: include/support/itoa/itoa.h:72
+
+extern _LIBCPP_FUNC_VIS char* __u64toa(uint64_t __value, char* __buffer);
+extern _LIBCPP_FUNC_VIS char* __u32toa(uint32_t __value, char* __buffer);
----------------
We don't marke functions in the dylib with extern.
================
Comment at: include/support/itoa/itoa.h:76
+template <typename _Tp, typename = void>
+struct _LIBCPP_HIDDEN __traits
+{
----------------
Maybe a more descriptive name than `__traits`?
================
Comment at: include/support/itoa/itoa.h:81
+#if __has_builtin(__builtin_clzll)
+ static _LIBCPP_INLINE_VISIBILITY auto width(_Tp __v) -> int
+ {
----------------
`width`, `convert` and `pow` need to be reserved names.
================
Comment at: include/support/itoa/itoa.h:123
+inline _LIBCPP_INLINE_VISIBILITY bool
+__mul_overflowed(unsigned char __a, _Tp __b, unsigned char& __r)
+{
----------------
`__builtin_mul_overflow` works for `char` and `short`, so can't we just call that?
================
Comment at: include/support/itoa/itoa.h:144
+ static_assert(is_unsigned<_Tp>::value, "");
+#if __has_builtin(__builtin_mul_overflow)
+ return __builtin_mul_overflow(__a, __b, &__r);
----------------
GCC provides these builtins but `__has_builtin` won't detect them.
We can probably safely assume that every compiler except `_LIBCPP_COMPILER_MSVC` provides them, and only work around MSVC.
================
Comment at: include/support/itoa/itoa.h:161
+template <typename _Tp>
+struct _LIBCPP_HIDDEN traits : __traits<_Tp>
+{
----------------
`traits` needs to be a reserved name, and preferably a more descriptive one.
================
Comment at: include/support/itoa/itoa.h:188
+
+ template <typename _It1, typename _It2, class _Up>
+ static _LIBCPP_INLINE_VISIBILITY auto
----------------
What's wrong with using `std::inner_product`?
================
Comment at: src/support/itoa/itoa.cpp:1
+// -*- C++ -*-
+//===----------------------------------------------------------------------===//
----------------
This file should be renamed `charconv.cpp` and moved into the main source directory.
================
Comment at: src/support/itoa/itoa.cpp:19
+
+static const char cDigitsLut[200] = {
+ '0', '0', '0', '1', '0', '2', '0', '3', '0', '4', '0', '5', '0', '6', '0',
----------------
This should be `constexpr` to ensure correct initialization.
================
Comment at: src/support/itoa/itoa.cpp:35
+
+#define APPEND1(i) \
+ do \
----------------
Any reason these can't be `static` functions? The compiler should optimize them away nicely.
================
Comment at: test/std/utilities/charconv/charconv.from.chars/integral.pass.cpp:56
+ r = from_chars(s, s + sizeof(s), x);
+ LIBCPP_ASSERT(r.ec == std::errc{});
+ LIBCPP_ASSERT(r.ptr == s + 3);
----------------
`LIBCPP_ASSERT` is only intended to test non-standard behavior that libc++ happens to have but that isn't required by the standard.
If this behavior is specified by the standard it should be tested using `assert` instead.
Repository:
rCXX libc++
https://reviews.llvm.org/D41458
More information about the cfe-commits
mailing list