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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
