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