Quuxplusone added a comment.
In https://reviews.llvm.org/D48753#1157695, @EricWF wrote:
> Why are we doing this?
>
> I can't find the language in the C++03 specification that requires us to call
> an allocators `construct` method if it's present.
I think it's being proposed under "quality of implementation."
It has only just now occurred to me: a quality implementation should probably
never //mismatch// calls to `construct` and `destroy`. Does libc++ currently
call `destroy` in C++03? Does it call `destroy` in C++03 after this patch? If
this patch is making libc++ call `construct` but not `destroy`, that's a bug.
If before this patch libc++ would call `destroy` but not `construct`, this
patch is a bugfix! (And please add a test case for matched pairs of
`construct` and `destroy`.)
================
Comment at: libcxx/include/memory:1470
+ decltype(_VSTD::declval<_Alloc>().construct(_VSTD::declval<_Pointer>(),
+ _VSTD::declval<_Args>())),
+ void
----------------
vsapsai wrote:
> Quuxplusone wrote:
> > vsapsai wrote:
> > > Quuxplusone wrote:
> > > > I think you should replace this `)))` with `)), void())` for absolute
> > > > correctness (e.g. `construct` might plausibly return a pointer to the
> > > > constructed object, and I think C++03 is okay with that).
> > > > Otherwise, awesome, this looks like what I'd expect. :)
> > > The standard says the result of `construct` is not used but I haven't
> > > found any mention it **must** be `void`.
> > >
> > > That being said, the proposed change fails for the C++03 macro-based
> > > decltype shim:
> > >
> > > error: too many arguments provided to function-like macro invocation
> > > _VSTD::declval<_Args>()), void()),
> > > ^
> > >
> > > One option to fix that is to use
> > >
> > > is_same
> > > <
> > > decltype(_as_before_),
> > > decltype(_the_same_copy_pasted_argument_)
> > > >
> > >
> > > But I don't think this use case is worth such hacks.
> > I think the workaround is easier. Try
> >
> > ```
> > template <class _Alloc, class _Pointer, class _A0>
> > struct __has_construct<_Alloc, _Pointer, _A0, typename enable_if<
> > is_same<
> > decltype((_VSTD::declval<_Alloc>().construct(
> > _VSTD::declval<_Pointer>(),
> > _VSTD::declval<_A0>()
> > ), void() )),
> > void
> > >::value
> > >::type> : std::true_type {};
> > ```
> >
> > i.e. with an extra set of parens to group the argument to the `decltype`
> > macro: `decltype(())` instead of `decltype()`. I also drive-by edited
> > `Args` to `A0`, matching how it's done elsewhere in
> > pseudo-variadic-templates in this file.
> Thanks for the suggestion, Arthur, that worked.
>
> Regarding `_Args` vs `_A0`, I am deliberately using `_Args` because it's not
> a manual pseudo-variadic-template expansion and it's not inside
> `_LIBCPP_HAS_NO_VARIADICS` block. But I don't feel strong about it, can
> change to `_A0`.
Please change it (as far as I'm concerned, anyway). "`_Args`" plural implies a
pack, and there's no pack here.
https://reviews.llvm.org/D48753
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
