[libcxx-commits] [PATCH] D96742: [libcxx] adds concept `std::assignable_from`

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb added a comment.

In D96742#2575681 <https://reviews.llvm.org/D96742#2575681>, @ldionne wrote:

> In D96742#2575636 <https://reviews.llvm.org/D96742#2575636>, @cjdb wrote:
>
>> Agreed. I'll add a few for `vector<int>` and `string`. Any other commonly used types that we should care about (e.g. `map`) or are `vector` and `string` "good enough"?
>
> The more the better, IMO. `map`, `deque`, `unique_ptr`, `variant` (?), just a few of the common ones. I doubt it'll actually help us find any issues with the concepts themselves, but it could potentially find us issues with those types!

I think it's a great idea, and one I'm happy to take on; but if we do it right now, any latent bugs in other types that we uncover will become blockers for getting the rest of the concepts merged into libc++ (see my recent email <https://lists.llvm.org/pipermail/libcxx-dev/2021-February/001081.html> for a potentially large blocker). I propose making a separate patch for each concept where we test all the frequently used standard types. Those patches can be indefinitely blocked, which isn't great, but at least it doesn't prevent libc++ from being C++20 feature-complete in a reasonable amount of time.

To reiterate: I'm not shrugging off this work, but I'd like to defer it to after `__cpp_lib_concepts` is defined. If someone is motivated to pick it up before then, feel free to do so: I can confirm at least one major issue will fall out of `std::ranges::swap`.



================
Comment at: libcxx/test/std/concepts/lang/assignable.compile.pass.cpp:21
+constexpr bool ModelsAssignableFrom() {
+  static_assert(!std::assignable_from<T1, T2>);
+  static_assert(!std::assignable_from<T1, const T2>);
----------------
ldionne wrote:
> Should we use the trick that Casey mentioned in another patch where we basically test `static_assert(std::assignable_from<T1, const T2> == std::assignable_from<T1, T2>);`?
> 
> That way, you could use something like this below: `static_assert(!ModelsAssignableFrom<int*&, const int*>());`
(Note: this is from D88131).

I thought I was emulating Casey's stuff, but seems it can be improved :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96742