Page MenuHomePhabricator

[libcxx] adds concept `std::assignable_from`
ClosedPublic

Authored by cjdb on Feb 15 2021, 6:59 PM.

Details

Summary

Implements parts of:

  • P0898R3 Standard Library Concepts
  • P1754 Rename concepts to standard_case for C++20, while we still can

Depends on D96660

Diff Detail

Event Timeline

cjdb requested review of this revision.Feb 15 2021, 6:59 PM
cjdb created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 15 2021, 6:59 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Mordante added inline comments.Feb 16 2021, 9:57 AM
libcxx/include/concepts
218

Why do you deviate from the wording in the standard? _Rhs __rhs versus _Rhs&& __rhs.

cjdb updated this revision to Diff 324180.Feb 16 2021, 8:56 PM

fixes the (very important) typo @Mordante pointed out

cjdb marked an inline comment as done.Feb 16 2021, 8:56 PM
cjdb added inline comments.
libcxx/include/concepts
218

Whoops! Thanks for catching this :-)

The implementation matches the spec closely. I didn't look at the tests in careful detail, however as a general high-level comment: is there value in testing these concepts on a few common types like std::string, std::vector, etc. just to make sure they behave as we'd expect them?

Those are the first types that users are going to use concepts with, so I tend to see value in making sure they work as expected (not that I see any reason they wouldn't, but catching bad surprises is the whole point). Basically, it's as if we had a few compilable examples of how to use the Standard Library.

libcxx/test/std/concepts/lang/assignable.compile.pass.cpp
22

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*>());

cjdb marked an inline comment as done.Feb 19 2021, 2:17 PM

The implementation matches the spec closely. I didn't look at the tests in careful detail, however as a general high-level comment: is there value in testing these concepts on a few common types like std::string, std::vector, etc. just to make sure they behave as we'd expect them?

Those are the first types that users are going to use concepts with, so I tend to see value in making sure they work as expected (not that I see any reason they wouldn't, but catching bad surprises is the whole point). Basically, it's as if we had a few compilable examples of how to use the Standard Library.

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"?

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!

cjdb added a comment.Feb 20 2021, 12:08 PM

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 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.

libcxx/test/std/concepts/lang/assignable.compile.pass.cpp
22

(Note: this is from D88131).

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

cjdb updated this revision to Diff 325680.Feb 22 2021, 10:26 PM
cjdb marked an inline comment as done.

applies @ldionne's feedback

cjdb added a comment.Feb 22 2021, 10:27 PM

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 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.

I decided it's better to do it here and now, so done.

cjdb updated this revision to Diff 326589.Feb 25 2021, 8:36 PM

isolates std::mutex

ldionne accepted this revision.Mar 2 2021, 9:15 AM

[...]

I decided it's better to do it here and now, so done.

When we encounter these issues, I think we can disable the failing tests with a comment saying "re-enable once <bad issue> is fixed". At least the structure of the tests will be there, so it'll be easy to come back and fix it.

This revision is now accepted and ready to land.Mar 2 2021, 9:15 AM

Pending CI, of course. I think CI might be failing because it's trying to apply your patch without applying the pre-requisites.

cjdb updated this revision to Diff 327584.Mar 2 2021, 2:00 PM

rebases to activate CI

cjdb updated this revision to Diff 327979.Mar 3 2021, 5:59 PM

rebases to activate CI

cjdb updated this revision to Diff 327985.Mar 3 2021, 6:32 PM

rebases to activate CI (bad gateway)

This revision was automatically updated to reflect the committed changes.

LGTM mod nits. (Seems like too many tests. The code change itself looks like a correct transliteration of the paper standard.)
aaand whoop, it's already landed :)

libcxx/test/std/concepts/lang/assignable.compile.pass.cpp
138

This auto means bool, right? I'd like to see bool here if that's the intention.
(ditto throughout)

547

Here and line 535: is it actually cromulent to create a container with a const (key/element) type? If this is sort of "library IFNDR" then we shouldn't be doing it in tests. But I don't think anything of value would be lost by removing these two lines, anyway, right?

ldionne added inline comments.Mar 4 2021, 3:39 PM
libcxx/test/std/concepts/lang/assignable.compile.pass.cpp
138

Nitpicky but +1, I'm not a huge fan of using auto for simple types.

547

Hmm, I had missed that indeed but std::vector<T const> isn't allowed because T const isn't movable. I think we should remove it. @cjdb did you do this in other tests too? If so, we should fix them too.

cjdb added inline comments.Mar 4 2021, 10:00 PM
libcxx/test/std/concepts/lang/assignable.compile.pass.cpp
138

Sure, I'll get a patch ready.

547

I'm a bit confused as to why we say that std::vector<T const> isn't allowed. It shouldn't be because T const isn't movable: that'd mean std::vector<std::mutex> is also disallowed (and as frustrating as it is to use, that type works "fine").

Quuxplusone added inline comments.Mar 5 2021, 12:48 PM
libcxx/test/std/concepts/lang/assignable.compile.pass.cpp
547

FWIW: I've looked at similar issues before (sadly don't remember exactly which containers — probably set or map) where ultimately the pragmatic problem was that two member functions ended up with the same signature — e.g. if you had void foo(iterator) and void foo(const_iterator), and both of those types were const T* (or iteratorImpl<const T> or something), then boom, collision. I don't know if formal wording exists to make it ill-formed. I do agree with you that vector<[non-const] mutex> is supported in practice; I'm only objecting to explicitly cv-qualified element types.