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

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added inline comments.


================
Comment at: include/charconv:89
+_LIBCPP_BEGIN_NAMESPACE_STD
+
+enum class _LIBCPP_ENUM_VIS chars_format
----------------
lichray wrote:
> 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.
> But this header is backported to C++11, so I intended to not to guard it.

In general, we don't provide new features for old language versions.

The only time we've ever done that is for `string_view`, and I'm **still** not sure I did the right thing there.


================
Comment at: src/charconv.cpp:34
+
+#define APPEND1(i)                              \
+    do                                          \
----------------
I'd really like to see some numbers here showing how this (somewhat awkward) code performs better than the straightforward versions.

Because maintenance is forever.

Note: I'm sure that this was true on older compilers - but is it true today?


================
Comment at: test/support/charconv_test_helpers.h:24
+
+template <typename To, typename From>
+constexpr auto
----------------
If this is supposed to be a C++17 or later header (and I'm pretty sure it is), you should add a `static_assert(TEST_STD_VER > 14, "");` to this header.



Repository:
  rCXX libc++

https://reviews.llvm.org/D41458