This is an archive of the discontinued LLVM Phabricator instance.

[libcxx][ranges] adds concept `sized_range` and cleans up `ranges::size`
ClosedPublic

Authored by cjdb on May 13 2021, 12:36 PM.

Details

Summary
  • adds sized_range and conformance tests
  • moves disable_sized_range into namespace std::ranges
  • removes explicit type parameter

Implements part of P0896 'The One Ranges Proposal'.

Diff Detail

Event Timeline

cjdb requested review of this revision.May 13 2021, 12:36 PM
cjdb created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMay 13 2021, 12:36 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
zoecarver accepted this revision as: zoecarver.May 13 2021, 4:21 PM

LGTM.

libcxx/include/__ranges/concepts.h
14

How was this working before?

libcxx/test/std/ranges/range.req/range.sized/sized_range.compile.pass.cpp
24

Rvalue array?

cjdb marked 2 inline comments as done.May 13 2021, 4:26 PM
cjdb added inline comments.
libcxx/include/__ranges/concepts.h
14

🤷

libcxx/test/std/ranges/range.req/range.sized/sized_range.compile.pass.cpp
24

That's int[5].

libcxx/test/std/ranges/range.req/range.sized/sized_range.compile.pass.cpp
24

I assume Zoe was asking for xvalue (rvalue-reference) to array: int (&&)[5] and int (&&)[].

libcxx/test/std/strings/string.view/range_concept_conformance.compile.pass.cpp
31–32

Unrelated to this PR, but it strikes me as odd that string_view is not a std::ranges::view.
Should you perhaps be asserting stdr::view<std::string_view&> instead?

cjdb marked 3 inline comments as done.May 13 2021, 9:12 PM
cjdb added inline comments.
libcxx/test/std/ranges/range.req/range.sized/sized_range.compile.pass.cpp
24

This test checks that an lvalue array is a sized range and that an rvalue array is too. I'm not sure what adding a test to check an rvalue reference to an array is going to add overall that int[5] doesn't already do.

libcxx/test/std/strings/string.view/range_concept_conformance.compile.pass.cpp
31–32

That would be because we haven't enabled string_view as a view yet.

zoecarver added inline comments.May 18 2021, 12:05 PM
libcxx/test/std/strings/string.view/range_concept_conformance.compile.pass.cpp
31–32

Hmm. I think we should at least have a comment saying that this test is wrong. I'd even say we should comment out the static_assert too (and make it positive).

Mordante accepted this revision.May 18 2021, 12:26 PM

LGTM!

libcxx/include/__ranges/concepts.h
9–10

Not part of your changes, but can you use a triple underscore? _LIBCPP_RANGES_CONCEPTS_H -> _LIBCPP___RANGES_CONCEPTS_H

libcxx/test/std/ranges/range.req/range.sized/sized_range.compile.pass.cpp
50

Why is this the only one to check stdr::range?

This revision is now accepted and ready to land.May 18 2021, 12:26 PM
libcxx/test/std/strings/string.view/range_concept_conformance.compile.pass.cpp
31–32

@zoecarver: If we comment out the assert, we'll have to set some kind of watchdog to remind us to comment it back in, in the PR that enables string_view-as-a-view. Which will definitely fail to remind us, and then tech debt.
By keeping this "wrong" assert enabled, it serves as a regression test: it serves as its own infallible watchdog. When string_view becomes a view, buildkite will point directly at this test file, telling us that we need to flip the sense of the assert as part of the same PR. That's excellent. (And also excellent to see in the git history later, when we're wondering which commit caused string_view to become a view.)
I wouldn't mind seeing a // TODO: string_view is nonconforming comment at the end of this line, though.

A possible alternative approach (but I assume we won't block D102434 on this): first land a PR that makes string_view-a-view; then land this PR with the assert already passing.

cjdb marked an inline comment as done.May 18 2021, 4:44 PM
cjdb added inline comments.
libcxx/include/__ranges/concepts.h
9–10

Will do.

14

Oh, because of transitive includes.

libcxx/test/std/ranges/range.req/range.sized/sized_range.compile.pass.cpp
50

Confirming it's a range, but not a sized range.

libcxx/test/std/strings/string.view/range_concept_conformance.compile.pass.cpp
31–32

@zoecarver: If we comment out the assert, we'll have to set some kind of watchdog to remind us to comment it back in, in the PR that enables string_view-as-a-view. Which will definitely fail to remind us, and then tech debt.
By keeping this "wrong" assert enabled, it serves as a regression test: it serves as its own infallible watchdog. When string_view becomes a view, buildkite will point directly at this test file, telling us that we need to flip the sense of the assert as part of the same PR. That's excellent. (And also excellent to see in the git history later, when we're wondering which commit caused string_view to become a view.)

+1

I wouldn't mind seeing a // TODO: string_view is nonconforming comment at the end of this line, though.

A possible alternative approach (but I assume we won't block D102434 on this): first land a PR that makes string_view-a-view; then land this PR with the assert already passing.

Sure, I can add that TODO before merging (it won't appear on Phab though).

This revision was automatically updated to reflect the committed changes.
cjdb marked an inline comment as done.