This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by cjdb on Feb 14 2021, 8:01 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 14 2021, 8:01 PM
cjdb created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 14 2021, 8:01 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
miscco added inline comments.Feb 14 2021, 11:51 PM
libcxx/test/std/concepts/lang/common.compile.pass.cpp
287

I would greatly prefer if we could give those types names that describe what we are checking rather than numbers where one has to read the source an the standard

cjdb added inline comments.Feb 15 2021, 10:43 AM
libcxx/test/std/concepts/lang/common.compile.pass.cpp
287

The interesting information is in all the CommonTypeXs. Tn really just facilitates the common types.

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

rebases to activate CI

ldionne added inline comments.Feb 19 2021, 2:26 PM
libcxx/include/concepts
195

I see this follows the spec closely, but I'm curious to understand. Here, I would have expected the check to be:

common_reference_with<
    common_type_t<_Tp, _Up> const&,
    common_reference_t<
        const _Tp&,
        const _Up&>>;

instead (with add_lvalue_reference_t simplified naively for legibility). In other words, why are we looking for a common reference between the non-const common type and the common-ref between the const-qualified types? I'm sure that'll highlight a misunderstanding I have about the concept.

libcxx/test/std/concepts/lang/common.compile.pass.cpp
20

Same comment about testing with static_assert(std::common_with<T, U&> == std::common_with<T, U>) as in the other patch, if you think it makes sense.

258

I'm not sure I follow this. Can you explain?

cjdb updated this revision to Diff 325380.Feb 21 2021, 11:43 PM
cjdb marked an inline comment as done.

applies some feedback

libcxx/include/concepts
195

In other words, why are we looking for a common reference between the non-const common type and the common-ref between the const-qualified types?

This asks the question "does common_type_t<T, U>& have a reference in common with common_reference_t<T const&, U const&>?" common_reference_t<T const&, U const&> should fall out as a reference-to-const unless basic_common_reference dictates otherwise, so it's essentially checking something to the effect of common_reference_with<CT&, CR const&>.

As for why this is the case, it screams proxy types to me (you wouldn't notice that unless you've played with zip_view). This is about as far as my knowledge goes though.

@CaseyCarter will probably correct me here, but getting stuff wrong on the Internet is how one learns?

libcxx/test/std/concepts/lang/common.compile.pass.cpp
20

Yep!

287

T8 and T9 were respectively renamed to CommonWithInt and CommonWithIntButRefLong, but I'm still not convinced about T1 through T7 (for now!)

ldionne accepted this revision.Feb 25 2021, 10:32 AM

LGTM except nitpick about formatting in the tests. No need to wait for my re-review.

libcxx/include/concepts
195

I'm still curious to know the answer to this (@CaseyCarter maybe?), but this shouldn't hold up the review.

libcxx/test/std/concepts/lang/common.compile.pass.cpp
527–537

I think these specializations would be glance over if you disregarded clang-format and defined them on a single line like

...
template <> struct common_type<T7&, int&> { using type = void; };
...
This revision is now accepted and ready to land.Feb 25 2021, 10:32 AM
cjdb updated this revision to Diff 327577.Mar 2 2021, 1:54 PM

rebases to activate CI

cjdb updated this revision to Diff 327977.Mar 3 2021, 5:55 PM

rebases to activate CI

This revision was automatically updated to reflect the committed changes.