Page MenuHomePhabricator

[libc++][ranges] Implement `std::sortable`.
ClosedPublic

Authored by var-const on Feb 11 2022, 10:36 PM.

Details

Reviewers
Quuxplusone
ldionne
Group Reviewers
Restricted Project
Commits
rG8e979460bb27: [libc++][ranges] Implement `std::sortable`.

Diff Detail

Event Timeline

var-const created this revision.Feb 11 2022, 10:36 PM
var-const requested review of this revision.Feb 11 2022, 10:36 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 11 2022, 10:36 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript

Use the actual patch link.

Quuxplusone requested changes to this revision.Feb 13 2022, 8:11 PM
Quuxplusone added a subscriber: Quuxplusone.

LGTM except for the tests. Thanks for working on this — you're very close to completing all the iterator concepts! :)

libcxx/include/__iterator/sortable.h
31–32

As usual, I'd prefer 2-space indents, even though clang-format disagrees.

libcxx/test/std/iterators/iterator.requirements/alg.req.sortable/sortable.compile.pass.cpp
16

#include <functional> for ranges::less (which I guess isn't used right now, but probably should be)

34

It occurs to me that you're missing tests for when _Comp is not ranges::less and/or _Proj is not std::identity.
Then, looking closer at this test, all sorts of problems pop into view.

static_assert( std::indirect_strict_weak_order<CompInt, AllConstraintsSatisfied>);
static_assert( std::sortable<AllConstraintsSatisfied>);

Maybe you meant either-or-both of these instead?

static_assert( std::indirect_strict_weak_order<ranges::less, AllConstraintsSatisfied>);
static_assert( std::sortable<AllConstraintsSatisfied>);
static_assert( std::indirect_strict_weak_order<CompInt, AllConstraintsSatisfied>);
static_assert( std::sortable<AllConstraintsSatisfied, CompInt>);

And then on line 32,

static_assert(!std::indirect_strict_weak_order<CompInt, NoIndirectStrictWeakOrder>);

is true, but has nothing to do with why !std::sortable<NoIndirectStrictWeakOrder>.

So, some more work is needed on this test.
This might also cause retroactive changes to mergeable.compile.pass.cpp, assuming it's missing the same kinds of coverage.

This revision now requires changes to proceed.Feb 13 2022, 8:11 PM
var-const updated this revision to Diff 409553.Feb 17 2022, 2:16 AM
var-const marked 3 inline comments as done.

Address feedback, rebase on main, guard sortable on NO_INCOMPLETE_RANGES
(because ranges::less is now also guarded on it).

var-const added inline comments.Feb 17 2022, 2:17 AM
libcxx/include/__iterator/sortable.h
31–32

Thinking more about this, I can buy an argument in favor of a 2-space indent for concepts. The general spirit is to use a narrower indent for continuation of constructs that are expected to normally be multiline (e.g. a function body) and a wider, more noticeable indent for constructs that are normally a single line but had to be split into multiple lines. For concepts, clearly the former is more applicable than the latter. Thus, treating the && and || more like ; in a function seems reasonable in this case.

libcxx/test/std/iterators/iterator.requirements/alg.req.sortable/sortable.compile.pass.cpp
34

Should be fixed, PTAL.

ldionne accepted this revision.Feb 17 2022, 10:30 AM
ldionne added a subscriber: ldionne.

LGTM except for the missing coverage for projections. I don't think I need to see this again.

libcxx/test/std/iterators/iterator.requirements/alg.req.sortable/sortable.compile.pass.cpp
28

You seem to be missing coverage for passing a projection. I think this also means you won't be able to use int if you want the projection to be non-trivial. You could define something like struct Point { int x; int y; }; and have your projection be a function Point -> int.

var-const updated this revision to Diff 409826.Feb 17 2022, 5:34 PM

Test a non-default projection.

Quuxplusone accepted this revision.Feb 17 2022, 6:08 PM
Quuxplusone added inline comments.
libcxx/test/std/iterators/iterator.requirements/alg.req.sortable/sortable.compile.pass.cpp
39

I think it would be useful (in the sense of self-documentation) to add

static_assert(!std::sortable<Foo*, CompInt>);
This revision is now accepted and ready to land.Feb 17 2022, 6:08 PM
var-const updated this revision to Diff 409832.Feb 17 2022, 6:37 PM
var-const marked 2 inline comments as done.

For non-default projections, assert that using the default projection doesn't work.

This revision was automatically updated to reflect the committed changes.