Page MenuHomePhabricator

[libcxx] fixes `common_reference` requirement for `swappable_with`
ClosedPublic

Authored by cjdb on Apr 2 2021, 2:36 PM.

Details

Summary

LWG3175 identifies that the common_reference requirement for
swappable_with is over-constraining and doesn't need to concern itself
with cv- or reference qualifiers.

Diff Detail

Event Timeline

cjdb requested review of this revision.Apr 2 2021, 2:36 PM
cjdb created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptApr 2 2021, 2:36 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Quuxplusone accepted this revision as: Quuxplusone.Apr 2 2021, 5:56 PM
Quuxplusone added a subscriber: Quuxplusone.

LGTM % nits.

libcxx/test/std/concepts/concepts.lang/concept.swappable/swappable_with.compile.pass.cpp
671

FWIW, you could just capitalize the letter p on line 680 and then you wouldn't need this function.

681

As-IMO-always with constexpr tests, it'd be better style to write

assert(a1.m == -5 && a2.m == 5);
return true;

This would incidentally match the snippet in https://cplusplus.github.io/LWG/issue3175 a little closer, too, FWIW.

However, maybe a bigger surprise with this test is that it's all constexpr. LWG3175 tests the non-compile-time case, and doesn't mention the constexpr keyword at all. Either way is probably equivalent for testing purposes, but this gets back to my usual "cruft" complaint: there's a lot of keywords not pulling their weight in here. ;)

684

Please add a "simple test" too. I think the simple test is just

static_assert(std::swappable_with<N::Proxy, N::A>);
static_assert(std::swappable_with<N::A, N::Proxy>);

(and if you wanted to eliminate the useless namespace N, that'd be cool; but I imagine you don't, because it's part of the LWG3175 snippet)

cjdb updated this revision to Diff 335168.Apr 4 2021, 12:52 PM

gets CI passing

libcxx/test/std/concepts/concepts.lang/concept.swappable/swappable_with.compile.pass.cpp
671

I found it surprising that [concept.swappable] did this, so I preserved it.

681

As-IMO-always with constexpr tests, it'd be better style to write

assert(a1.m == -5 && a2.m == 5);
return true;

Do you have a technical reason for preferring this?

LWG3175 tests the non-compile-time case, and doesn't mention the constexpr keyword at all.

Eh, I suppose I could make this a run-time test if you insist, but the regression test is aimed at the concept std::swappable_with, not std::ranges::swap.

but this gets back to my usual "cruft" complaint: there's a lot of keywords not pulling their weight in here. ;)

I don't see the connection, but I also don't see which keywords aren't doing their job.

684

The crux of LWG3175 is that Proxy and A don't share a common reference (and so swappable_with<Proxy, A> == false). The defect was originally filed to correct the example only, and IIRC Casey noticed that the common_reference requirement was also out of whack.

I'm not sure there is a companion simple test for this, but I'm happy to be proven wrong.

Please improve commit title and add a proper commit message to the review.
I also like to see it pass the CI.

libcxx/test/std/concepts/concepts.lang/concept.swappable/swappable_with.compile.pass.cpp
681

As-IMO-always with constexpr tests, it'd be better style to write

assert(a1.m == -5 && a2.m == 5);
return true;

Do you have a technical reason for preferring this?

I personally like the style with separate asserts better, especially if it gets more complex. For example I dislike https://reviews.llvm.org/D97115#inline-910325, but assert can't be used in C++11.

In this case I would like it to better match the wording of LWG3175.

cjdb updated this revision to Diff 335370.Apr 5 2021, 7:03 PM
cjdb retitled this revision from [libcxx] applies LWG3175 to [libcxx] fixes `common_reference` requirement for `swappable_with`.
cjdb edited the summary of this revision. (Show Details)

adds assert and makes commit message more descriptive

cjdb marked 2 inline comments as done.Apr 5 2021, 7:03 PM
cjdb added inline comments.
libcxx/test/std/concepts/concepts.lang/concept.swappable/swappable_with.compile.pass.cpp
681

I don't like assert in constexpr diagnostics, but matching LWG3175 code is probably best.

curdeius accepted this revision.Apr 6 2021, 12:32 AM

LGTM.

This revision is now accepted and ready to land.Apr 6 2021, 12:32 AM
cjdb updated this revision to Diff 335559.Apr 6 2021, 9:47 AM
cjdb marked an inline comment as done.

rebases to activate CI (hopefully for the last time)