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

[Zhihao Yuan via Phabricator via cfe-commits]

lichray added inline comments.


================
Comment at: include/charconv:89
+_LIBCPP_BEGIN_NAMESPACE_STD
+
+enum class _LIBCPP_ENUM_VIS chars_format
----------------
mclow.lists wrote:
> 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.
We need to decide on this... From my point of view this header will be widely used by formatting and logging libraries, and it doesn't add much to the community by enforcing C++17 here, especially when the interface we specified are very simple and not using any features beyond C++11.


================
Comment at: src/charconv.cpp:34
+
+#define APPEND1(i)                              \
+    do                                          \
----------------
mclow.lists wrote:
> 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?
One thing I could try may be using return values rather than output parameters, like
```
buffer = append3(buffer, b); 
```
But I really don't think doing these makes the code any bit more maintainable comparing to
```
APPEND3(b);
```
, which looks very straightforward and expressive to me...


Repository:
  rCXX libc++

https://reviews.llvm.org/D41458