mclow.lists added a comment.
Getting close here. I'll have a couple more comments later today, so don't
post a new diff quite yet.
================
Comment at: include/charconv:244
+ static _LIBCPP_INLINE_VISIBILITY char const*
+ read(char const* __p, char const* __ep, type& __a, type& __b)
+ {
----------------
Quuxplusone wrote:
> mclow.lists wrote:
> > lichray wrote:
> > > mclow.lists wrote:
> > > > Same comment as above about `read` and `inner_product` - they need to
> > > > be "ugly names"
> > > Unlike `traits` which is a template parameter name in the standard,
> > > `read` and `inner_product` are function names in the standard, which
> > > means the users cannot make a macro for them (and there is no guarantee
> > > about what name you make **not** get by including certain headers), so we
> > > don't need to use ugly names here, am I right?
> > I understand your reasoning, but I don't agree.
> >
> > Just last month, I had to rename a function in `vector` from `allocate` to
> > `__vallocate` because it confused our "is this an allocator" detection. The
> > function in question was private, so it shouldn't have mattered, but GCC
> > has a bug where sometimes it partially ignores access restrictions in
> > non-deduced contexts, and then throws a hard error when it comes back to a
> > different context. The easiest workaround was to rename the function in
> > `vector`.
> >
> > Since then, I've been leery of public names that match others. This is
> > pretty obscure, since it's in a private namespace, but I'd feel better if
> > they were `__read` and `__inner_product`.
> >
> FWIW, +1 to ugly names. Even if the un-ugly code is "technically not broken
> yet", and besides the technical reason Marshall gives,
> (1) it's nice that libc++ has style rules and sticks to them, precisely to
> *avoid* bikeshedding the name of every private member in the world;
> (2) just because users can't `#define read write` doesn't mean they *won't*
> do it. I would actually be extremely surprised if `read` were *not* defined
> as a macro somewhere inside `<windows.h>`. :)
>
> See also: "should this function call be `_VSTD::`-qualified?" Sometimes the
> answer is technically "no", but stylistically "yes", precisely to indicate
> that we *don't* intend for it to be an ADL customization point. Consistent
> style leads to maintainability.
> See also: "should this function call be _VSTD::-qualified
We already have an `inner_product` ( in `<algorithm>`), so I think not.
We `_VSTD::` qualify `move` and `forward` always. `min`/`max` due mostly to
windows macros. `swap` when need to disable ADL.
Other things we're inconsistent about: `declval` sometimes (I don't know why),
`use_facet`, `addressof`, `to_raw_pointer`.
I'll figure out when we should mark things with `_VSTD` and write it up.
================
Comment at: include/charconv:359
+ auto __gen_digit = [](_Tp __c) {
+ return "0123456789abcdefghijklmnopqrstuvwxyz"[__c];
+ };
----------------
lichray wrote:
> mclow.lists wrote:
> > Thinking some more - did this used to do more? Because I don't see why
> > having a lambda here is a benefit, as compared to:
> >
> > const char *__digits = "0123456789abcdefghijklmnopqrstuvwxyz";
> >
> > and
> > *--p = digits[__c];
> >
> I use a lambda here because it may do more in the future. If someone wants
> to let it support non-ASCII platforms, then they only need to make a patch
> against this lambda rather than changing the sites of uses. After all, there
> is nothing wrong to abstract out anything into a function, I think...
I don't think that making it a lambda today buys us anything. Since it is
local, we can make it a lambda again in the future if we need to w/o breaking
anyone.
Shades of YAGNI.
================
Comment at: include/charconv:158
+
+#if !defined(_LIBCPP_COMPILER_MSVC)
+ static _LIBCPP_INLINE_VISIBILITY int __width(_Tp __v)
----------------
In general, we don't put `_LIBCPP_COMPILER_XXX` in header files in libc++.
Instead, we declare a feature macro `_LIBCPP_HAS_XXXX` in `<__config>` and then
use that everywhere.
I see that most of the uses of `_LIBCPP_COMPILER_MSVC` are in `<algorithm>`,
and center around the builtins for `__builtin_clz`, etc.
We can clean this up later. (/me makes note).
================
Comment at: include/charconv:285
+inline _LIBCPP_INLINE_VISIBILITY auto
+__to_unsigned(_Tp __x)
+{
----------------
in libc++, we put the return type on the left.
template <typename _Tp>
inline _LIBCPP_INLINE_VISIBILITY
auto __to_unsigned(_Tp __x)
(and then ask 'why auto'?)
================
Comment at: test/support/charconv_test_helpers.h:226
+void
+run(type_list<Ts...>)
+{
----------------
This name `run` concerns me. I'd feel better if it was `run_integral_tests` or
something a bit less general.
On the other hand, it's in a test, so we can change it whenever we want.
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
