[libcxx-commits] [PATCH] D101770: [libc++] Fix QoI bug with construction of std::tuple involving std::any

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision as: libc++.
ldionne added a comment.

In D101770#2736470 <https://reviews.llvm.org/D101770#2736470>, @Quuxplusone wrote:

> @ldionne: At first I didn't see why builtins were related, but I'm coming around to your point of view. Consider: https://www.godbolt.org/z/Y45bTadaq  (which oddly fails with libstdc++ but //not// with libc++!)
>
> Here, the compiler is trying to figure out whether `B` is copy-constructible. So one of the things it asks is whether the constructor `B(any)` is usable as a copy-constructor. So it asks whether `any` is convertible-from `const B&`. But //the compiler shouldn't ask that, because the answer is obviously irrelevant in this context!// Even if `any` //were// convertible-from `const B&`, that would be a user-defined conversion, which means we wouldn't be able to stack it with a second user-defined conversion `B(any)` in order to copy-construct a `B` object.  So indeed, I think the compiler is wrong to ask whether `any` is convertible-from `const B&`. Clang's and GCC's builtins have identical behavior, though.
>
> And there's a weird dependence on whether I use `is_copy_constructible<T>` versus `is_constructible<T, const T&>` in various places.  I guess that's because we need to be in the middle of instantiating `is_constructible<T, const T&>` when we hit a call to `is_copy_constructible<T>` — the symptom is "`is_copy_constructible` has an incomplete base class."
>
> And //that// implies that this would be a sufficient (although obviously not intellectually satisfying) fix for the original bug report:
>
>   diff --git a/libcxx/include/any b/libcxx/include/any
>   index aaeaab6c8f74..997f1656e157 100644
>   --- a/libcxx/include/any
>   +++ b/libcxx/include/any
>   @@ -204,7 +204,7 @@ public:
>        , class = enable_if_t<
>            !is_same<_Tp, any>::value &&
>            !__is_inplace_type<_ValueType>::value &&
>   -        is_copy_constructible<_Tp>::value>
>   +        is_constructible<_Tp, const _Tp&>::value>
>        >
>      _LIBCPP_INLINE_VISIBILITY
>      any(_ValueType && __value);
>
> I've tested it with your test case and it does make the test case work (but of course only the `std::any` test case, not your original hand-rolled `::any`).

Very interesting, thanks for the analysis. I thought it was an issue with the builtins, but I didn't do the work of figuring out why exactly. I think we should open a bug report against Clang - do you want to do it or should I?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101770/new/

https://reviews.llvm.org/D101770