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