[libcxx-commits] [PATCH] D116621: [libc++][P2321R2] Add const overloads to tuple swap, construction and assignment

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

Thank you! This is a complicated patch but it ends up simplifying our `tuple` implementation quite a bit. Thanks for the great testing coverage. too.



================
Comment at: libcxx/include/tuple:848
     struct _EnableImplicitCopyFromPair : _And<
-        is_constructible<_FirstType<_DependentTp...>, const _Up1&>,
-        is_constructible<_SecondType<_DependentTp...>, const _Up2&>,
----------------
huixie90 wrote:
> ldionne wrote:
> > huixie90 wrote:
> > > ldionne wrote:
> > > > EricWF wrote:
> > > > > These bits of SFINAE were written very specifically because tuple is a very weird beast.  
> > > > > 
> > > > > I'm surprised we don't have a test in the test suite that breaks after changing this, because I thought I added one.
> > > > > 
> > > > > There are certain SFINAE loops that crop up where checking these properties in the specific order is/was important.
> > > > > 
> > > > I think what Eric's saying here is that you need to test for `_EnableCopyFromPair` *before* you test the two `is_convertible` conditions.
> > > > 
> > > > I think you could witness the difference by using a type that is *not* constructible from `_Up`, but where a conversion from `_Up` triggers a hard error. In that case, the fact that you changed the order of checking will cause you to instantiate the conversion operator before the constructor, and explode. If you had checked for the constructibility first, you'd have gotten a false answer and stopped there, instead of being tripped up by the hard error later on while trying to check whether the constructor should be explicit or not.
> > > > 
> > > > Please add tests for this -- we actually see these sorts of funky bad-behaved constructors and conversions in the wild. I remember breaking `nlohmann::json` with a change a couple years ago -- it wasn't fun. I'm not sure whether it ended up being our fault our theirs, but regardless, we want to be bullet proof to avoid breaking widely used libraries, because we look like jokers when that happens :-).
> > > I fixed the ordering issue from the previous version. But I am not sure If I understand the test case. IIUC, you are suggesting a case where
> > > std::is_constructible_v<T, U> is false, but
> > > std::is_convertible_v<U, T> is hard error
> > > 
> > > IIUC, convertible is a stricter version of constructible. if is_convertible_v has a hard error, the conversion operator/constructor that causes the error would also cause a hard error when evaluating is_constructible. or did I miss something?
> > I think I was thinking about a type having a conversion operator that triggers a hard error. However, it doesn't seem to apply anymore, but it would be nice to ensure that we don't trip over a type `T` where `is_constructible<T, U>::value` is false, but where `is_convertible<T, U>::value` is a hard error (by defining a conversion operator from `U` to `T` that causes a hard error). Let me know if you think this doesn't make sense, it might be impossible to actually trigger this issue but I'd need to dive deeper to be certain.
> hmm. IIUC, `is_constructible` also looks for conversion operator. If a conversion operator from `U` to `T` causes a hard error, both `is_convertible` and `is_constructible` would cause hard error. or, did I miss anything?
I think you're right.


================
Comment at: libcxx/test/std/utilities/tuple/tuple.tuple/tuple.cnstr/alloc_non_const_pair.pass.cpp:112
+}
\ No newline at end of file

----------------
Can you please make sure you have newlines at the end of files?


================
Comment at: libcxx/test/std/utilities/tuple/tuple.tuple/tuple.swap/member_swap_const.pass.cpp:14
+// UNSUPPORTED: c++03, c++11, c++14, c++17, c++20
+// XFAIL: gcc
+
----------------
Why does it fail on GCC?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116621