This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by cjdb on Feb 13 2021, 11:50 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 D96657

Diff Detail

Event Timeline

cjdb requested review of this revision.Feb 13 2021, 11:50 PM
cjdb created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 13 2021, 11:50 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
cjdb updated this revision to Diff 323597.Feb 14 2021, 12:25 AM

corrects requirement

Mordante added inline comments.Feb 14 2021, 6:26 AM
libcxx/include/concepts
176

Minor nitpick the section is called [concept.commonref].

181

I know it behaves the same, but is there a reason you use common_reference_t<_Up, _Tp> instead of common_reference_t<_Tp, _Up> as specified in the standard?

libcxx/test/std/concepts/lang/commonreference.compile.pass.cpp
55

Can you guard this with #ifndef _LIBCPP_HAS_NO_INT128?

cjdb updated this revision to Diff 323629.Feb 14 2021, 11:43 AM
cjdb marked 2 inline comments as done.

applies @Mordante's feedback

cjdb marked an inline comment as done.Feb 14 2021, 11:43 AM
cjdb added inline comments.
libcxx/include/concepts
181

I think this was a late night change from common_reference_t<_Up, _Up>.

Mordante accepted this revision.Feb 14 2021, 11:50 AM

Thanks! Provided it passes CI, LGTM.

Do we need tests for indirections like std::reference_wrapper?

cjdb added a comment.Feb 15 2021, 10:43 AM

Do we need tests for indirections like std::reference_wrapper?

@miscco what would that buy us?

As far as I can tell the last clause from common_reference is specifically made for something like reference_wrapper types

As far as I can tell the last clause from common_reference is specifically made for something like reference_wrapper types

It's specifically for "Pre-LWG-2993 reference_wrapper"-like types; modern reference_wrapper doesn't need it. In fact the only test case I know of for that last clause is to implement pre-LWG-2993 reference_wrapper and check common_reference_t<old_reference_wrapper<meow>, meow&>.

cjdb updated this revision to Diff 324176.Feb 16 2021, 8:39 PM

rebases to active CI

cjdb added a comment.Feb 16 2021, 8:47 PM

As far as I can tell the last clause from common_reference is specifically made for something like reference_wrapper types

It's specifically for "Pre-LWG-2993 reference_wrapper"-like types; modern reference_wrapper doesn't need it. In fact the only test case I know of for that last clause is to implement pre-LWG-2993 reference_wrapper and check common_reference_t<old_reference_wrapper<meow>, meow&>.

@CaseyCarter does MSVC test this? I couldn't find it when I looked for an example.

As far as I can tell the last clause from common_reference is specifically made for something like reference_wrapper types

It's specifically for "Pre-LWG-2993 reference_wrapper"-like types; modern reference_wrapper doesn't need it. In fact the only test case I know of for that last clause is to implement pre-LWG-2993 reference_wrapper and check common_reference_t<old_reference_wrapper<meow>, meow&>.

@CaseyCarter does MSVC test this? I couldn't find it when I looked for an example.

https://github.com/microsoft/STL/blob/a31bd6ed6b4f1c18045bc329b38c08845ec0fc3d/tests/std/tests/VSO_0000000_type_traits/test.cpp#L1474-L1485

I don't see any major issues with patch. It looks OK to me.

ldionne accepted this revision.Feb 19 2021, 1:54 PM

Getting the easy stuff out of the way first, before I dive into common_reference proper. Thanks!

This revision is now accepted and ready to land.Feb 19 2021, 1:54 PM

(I assume we'll handle types behaving like the old reference_wrapper correctly in your common_reference patch)

cjdb updated this revision to Diff 327575.Mar 2 2021, 1:50 PM

rebase to activate CI

cjdb updated this revision to Diff 327652.Mar 2 2021, 7:02 PM

previous CI flaked, reactivating...

cjdb updated this revision to Diff 327933.Mar 3 2021, 2:51 PM

rebases to activate CI

This revision was automatically updated to reflect the committed changes.