[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: .gitignore:7
 *.so
+*.core
+
----------------
I'm pretty sure this file doesn't belong in this diff.


================
Comment at: include/charconv:89
+_LIBCPP_BEGIN_NAMESPACE_STD
+
+enum class _LIBCPP_ENUM_VIS chars_format
----------------
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.



================
Comment at: include/charconv:151
+
+#if __has_builtin(__builtin_clzll)
+    if (__tx::digits <= __diff || __tx::width(__value) <= __diff)
----------------
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.



================
Comment at: include/charconv:234
+to_chars(char* __first, char* __last, _Tp __value, int __base)
+    -> to_chars_result
+{
----------------
lichray wrote:
> mclow.lists wrote:
> > Why use the trailing return type here?
> > I don't see any advantage - it doesn't depend on the parameters (template or runtime).
> > 
> > 
> Because clang-format doesn't distinguish storage specifiers and simple-type-specifiers, and I want to format return types along with function signatures rather than letting hanging somewhere.
That's an argument for fixing clang-format, not for writing the code this way.



================
Comment at: include/charconv:240
+template <typename _Tp>
+struct _LIBCPP_HIDDEN traits : __traits_base<_Tp>
+{
----------------
I'm pretty sure we can't declare a type named `traits` - even inside a namespace.



================
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";
----------------
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.


Repository:
  rCXX libc++

https://reviews.llvm.org/D41458