[libcxx-commits] [PATCH] D121223: [libc++][NFC] Cleanups in <charconv>.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked 4 inline comments as done.
Mordante added a comment.

Thanks for the reviews!



================
Comment at: libcxx/include/charconv:107-108
 namespace __itoa {
-_LIBCPP_AVAILABILITY_TO_CHARS _LIBCPP_FUNC_VIS char* __u64toa(uint64_t __value, char* __buffer) _NOEXCEPT;
-_LIBCPP_AVAILABILITY_TO_CHARS _LIBCPP_FUNC_VIS char* __u32toa(uint32_t __value, char* __buffer) _NOEXCEPT;
+_LIBCPP_AVAILABILITY_TO_CHARS _LIBCPP_FUNC_VIS char* __u64toa(uint64_t __value, char* __buffer) noexcept;
+_LIBCPP_AVAILABILITY_TO_CHARS _LIBCPP_FUNC_VIS char* __u32toa(uint32_t __value, char* __buffer) noexcept;
 } // namespace __itoa
----------------
Quuxplusone wrote:
> philnik wrote:
> > I think this fails in C++03, but I don't know why it is available in C++03 at all.
> +1; could this be moved under the following `#if`?
Good point, we didn't support it in C++03, but missed the `#if` wasn't placed correctly.


================
Comment at: libcxx/include/charconv:171
 struct _LIBCPP_HIDDEN
     __traits_base<_Tp, decltype(void(uint32_t{declval<_Tp>()}))>
 {
----------------
Quuxplusone wrote:
> Pre-existing, tangential: I notice that here the SFINAE is testing explicit curly-brace-initialization from `_Tp` to `uint32_t`, but what's actually used on line 184 is implicit conversion (and ADL on `_Tp`). If `_Tp` is always a built-in integer type, this is probably all fine.
The condition `is_integral<_Tp>::value` is `true` when this code is used. So it should be fine.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121223/new/

https://reviews.llvm.org/D121223