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

[Zhihao Yuan via Phabricator via cfe-commits]

lichray marked 3 inline comments as done.
lichray added inline comments.


================
Comment at: include/charconv:89
+_LIBCPP_BEGIN_NAMESPACE_STD
+
+enum class _LIBCPP_ENUM_VIS chars_format
----------------
mclow.lists wrote:
> Quuxplusone wrote:
> > lichray wrote:
> > > 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.
> > This question is also relevant to my interests, in re `<memory_resource>`.
> > From my point of view this header will be widely used by formatting and logging libraries,
> 
> Non-portable formatting and logging libraries - if we provide it before C++17 and they use it.
> 
When they use it, what's next?

1. Someone try to use the library against libstdc++, he/she
   - file a bug report to the library, or
   - file a bug report to libstdc++
2. The library has too little users, so it's okay for it to be non-portable.

Looks good to me.


================
Comment at: include/charconv:151
+
+#if __has_builtin(__builtin_clzll)
+    if (__tx::digits <= __diff || __tx::width(__value) <= __diff)
----------------
mclow.lists wrote:
> lichray wrote:
> > EricWF wrote:
> > > `<algorithm>` already has a `__clz` wrapper for `__builtin_clz` et al. We should use that instead. That also allows us to get rid of the fallback implementation, and it correctly uses the builtin for compilers like GCC which don't provide `__has_builtin`.
> > I saw that, and I agree this can be improved, however `<algorithm>` would be too heavy to include here.  Thoughts?
> We could make this depend on `<bit>` once P0553 is adopted. (use `countl_zero`)
> 
>  My intention there is to provide `__countl_zero` etc for internal use before C++20, and the public names for C++20 and later.  That approach could be used here as well, if we choose.
> 
OK.


================
Comment at: test/std/utilities/charconv/charconv.from.chars/integral.pass.cpp:133
+        {
+            // If the pattern allows for an optional sign,
+            char s[] = " - 9+12";
----------------
mclow.lists wrote:
> Rather than attempting to reuse bits of the string here, I would define a struct:
> 
>     struct test_data {
>         const char *input;
>         const char *output;
>         std::errc err;
>         T value;
>         };
> 
> and then write a bunch of test cases and iterate over them.
No longer reusing parts from the string.


Repository:
  rCXX libc++

https://reviews.llvm.org/D41458