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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added a comment.

@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`).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101770