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

[Zhihao Yuan via Phabricator via cfe-commits]

lichray marked an inline comment as done.
lichray added inline comments.


================
Comment at: include/charconv:158
+
+#if !defined(_LIBCPP_COMPILER_MSVC)
+    static _LIBCPP_INLINE_VISIBILITY int __width(_Tp __v)
----------------
mclow.lists wrote:
> In general, we don't put `_LIBCPP_COMPILER_XXX` in header files in libc++.
> Instead, we declare a feature macro `_LIBCPP_HAS_XXXX` in `<__config>` and then use that everywhere.
> 
> I see that most of the uses of `_LIBCPP_COMPILER_MSVC` are in `<algorithm>`, and center around the builtins for `__builtin_clz`, etc.
> 
> We can clean this up later. (/me makes note).
> 
MSVC has the intrinsics we need, but I did not plan to put too much in this patch.  The suggested `<bits>` header sounds like a good place for grouping up those intrinsics in the future.


================
Comment at: include/charconv:285
+inline _LIBCPP_INLINE_VISIBILITY auto
+__to_unsigned(_Tp __x)
+{
----------------
mclow.lists wrote:
> in libc++, we put the return type on the left.
> 
>     template <typename _Tp>
>     inline _LIBCPP_INLINE_VISIBILITY
>     auto __to_unsigned(_Tp __x)
> 
> (and then ask 'why auto'?)
> 
```
template <typename _Tp>
inline _LIBCPP_INLINE_VISIBILITY
auto __to_unsigned(_Tp __x)
```

I want this as well, but ClangFormat doesn't support it... The style which I'm using is close to FreeBSD style(9), please forgive.  I don't mind if you send me a `.clang-format` file (the one I used is http://paste.openstack.org/show/726883/), but I won't manually adjust the code...

About why `auto` -- this file requires C++14 already, so I don't need to repeat my self (by writing `make_unsigned_t<_Tp>` again).


================
Comment at: test/support/charconv_test_helpers.h:226
+void
+run(type_list<Ts...>)
+{
----------------
mclow.lists wrote:
> This name `run` concerns me. I'd feel better if it was `run_integral_tests` or something a bit less general.
> 
> On the other hand, it's in a test, so we can change it whenever we want.
> 
The callsites look like
```
run<test_signed>(all_signed);
```
That's why I named it "run".  Not an issue for now I think.


Repository:
  rCXX libc++

https://reviews.llvm.org/D41458