This is an archive of the discontinued LLVM Phabricator instance.

[libcxx][ranges] Add `ranges::common_view`.
ClosedPublic

Authored by zoecarver on Jul 9 2021, 4:48 PM.

Details

Reviewers
cjdb
ldionne
Group Reviewers
Restricted Project
Commits
rGe5d8b93e5a25: [libcxx][ranges] Add `ranges::common_view`.

Diff Detail

Event Timeline

zoecarver created this revision.Jul 9 2021, 4:48 PM
zoecarver requested review of this revision.Jul 9 2021, 4:48 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 9 2021, 4:48 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
zoecarver updated this revision to Diff 357672.Jul 9 2021, 4:54 PM
  • Update module map.
  • Update synopsis.
  • Add correct enable_borrowed_range.
Quuxplusone added inline comments.
libcxx/include/__ranges/common_view.h
15–16

a-z plz (and throughout, if I missed any)

85

Nit: lose the _VSTD on declval, for consistency with other libc++ headers. (It's ADL-proof by virtue of taking no arguments.)
But also, why is this not

template<class _Range>
common_view(_Range&&)
  -> common_view<views::all_t<_Range>>;

as in the standard's reference implementation? If we have all, we must have all_t, right?

libcxx/include/ranges
126

Also update the modulemap.

libcxx/test/std/ranges/range.adaptors/range.common.view/base.pass.cpp
29
73

I suggest adding above line 72:

static_assert(!has_lvalue_qualified_base(common));

which could be implemented as

constexpr bool has_lvalue_qualified_base(auto&& view) {
    return requires { view.base(); };
}

This replaces your BaseEnabled, but puts the test in the salient place — right where we would expect to see the test for common.base(), you'd show the reason it's not tested. (This eliminates lines 59-63.)

libcxx/test/std/ranges/range.adaptors/range.common.view/begin.pass.cpp
81

/Randomc/Random/

94–99

I'd prefer to see these as hidden friends of RandomAccessIter, or know the technical reason why they can't be. Ditto lines 72-77 above.

116

Why do we need this if? What piece is not constexpr-friendly here?

libcxx/test/std/ranges/range.adaptors/range.common.view/ctad.compile.pass.cpp
37

I'd call this BorrowedRange. AIUI, the salient thing about a borrowed-range type is that it is borrowed, e.g. string_view. It's not merely borrow-able (like, I guess, string might be considered borrow-able).

47–56

Consider named variables, and test both lvalues and rvalues.

void test_ctad() {
    View v;
    Range r;
    BorrowedRange br;
    static_assert(std::same_as<
        decltype(std::ranges::common_view(v)),
        std::ranges::common_view<View>
    >);
    static_assert(std::same_as<
        decltype(std::ranges::common_view(r)),
        std::ranges::common_view<std::ranges::ref_view<Range>>
    >);
    // std::ranges::common_view(std::move(r)) is ill-formed
    static_assert(std::same_as<
        decltype(std::ranges::common_view(br)),
        std::ranges::common_view<std::ranges::ref_view<BorrowedRange>>
    >);
    static_assert(std::same_as<
        decltype(std::ranges::common_view(std::move(br))),
        std::ranges::common_view<std::views::all_t<BorrowedRange>>
    >);
}
zoecarver added inline comments.Jul 12 2021, 9:19 AM
libcxx/test/std/ranges/range.adaptors/range.common.view/begin.pass.cpp
94–99

RandomAccessIter is a type alias to random_access_iterator. I think we both agree we shouldn't add these as hidden friends to random_access_iterator.

116

Basically none of common_iterator is constexpr. Sigh.

libcxx/test/std/ranges/range.adaptors/range.common.view/begin.pass.cpp
94–99

Yes, although also it's not great to see them just hanging out in some random test file.
The next most stylish thing to do would be to make SizedRandomcAccessView [sic] have a nested type struct sentinel, and then make these hidden friends of SizedRandomcAccessView::sentinel. (I'm still rather unclear on why we have this generic "sentinel wrapper" type; it never seems to be quite what we want for testing.) But anyway, that's clearly too big a change for this PR, so I'm OK with leaving this hack for now.

116

I see. Ugh. The constructor and accessors of common_view are constexpr, but common_iterator isn't. OK.

zoecarver marked 10 inline comments as done.Jul 12 2021, 12:39 PM
zoecarver added inline comments.
libcxx/include/__ranges/common_view.h
85

We don't have all_t, it's on my list of things todo.

libcxx/include/ranges
126

It's already updated.

zoecarver marked 2 inline comments as done.Jul 12 2021, 12:40 PM

Apply Arthur's comments.

zoecarver marked an inline comment as done.Jul 12 2021, 12:41 PM
ldionne requested changes to this revision.Jul 13 2021, 2:18 PM

Per OTS review.

libcxx/test/std/ranges/range.adaptors/range.common.view/base.pass.cpp
59

Could you store the result of .base() in a variable just so we get an explicit assertion of what the return type of the function is?

libcxx/test/std/ranges/range.adaptors/range.common.view/begin.pass.cpp
72–77

Are those only for convenience? If so, they should be removed, otherwise, can you please add a one-liner comment about why they are needed? (same below)

113

We should not be testing begin() by dereferencing the resulting iterator. Instead, we should compare the iterator itself. Is that possible or am I missing something?

116

Instead of doing this if (!is_constant_evaluated()), I would make the test non-constexpr and add a simpler constexpr test that only calls begin() (const and non-const) in a constexpr context.

Then, when we go back and make common_iterator constexpr-friendly, we'll only have to remove that tiny test and make the whole test constexpr.

libcxx/test/std/ranges/range.adaptors/range.common.view/borrowing.compile.pass.cpp
19

Not needed. (might apply elsewhere too)

libcxx/test/std/ranges/range.adaptors/range.common.view/ctad.compile.pass.cpp
58

Is it ill-formed, or should it SFINAE away? I think it's the latter, right? If so, we can use a requires to check it.

libcxx/test/std/ranges/range.adaptors/range.common.view/ctor.pass.cpp
13–14

I would like to split this into ctor.default.pass.cpp and ctor.view.pass.cpp for consistency w/ the rest of the test suite.

libcxx/test/support/test_iterators.h
927

constexpr I const& base() const?

This revision now requires changes to proceed.Jul 13 2021, 2:18 PM
zoecarver marked 7 inline comments as done.Jul 15 2021, 11:10 AM

Apply Louis' comments.

zoecarver marked an inline comment as done.
  • Global constexpr friend -> friend constexpr.
zoecarver marked an inline comment as done.Jul 15 2021, 11:14 AM
ldionne requested changes to this revision.Jul 20 2021, 12:44 PM

Please make sure you go through https://libcxx.llvm.org/Contributing.html (I think you've got everything except the visibility macros, which I added recently to the list, but it might be worth double-checking).

This basically LGTM. You can update this and ping me again, but it should be a no-brainer.

libcxx/include/__ranges/common_view.h
40

Missing _LIBCPP_HIDE_FROM_ABI everywhere. Also missing from common_iterator.h, sorry for not noticing earlier.

Note: Yes, that's a pain. If someone wants to work on solving this a better way, let me know, I've got some ideas with inline namespaces that might be worth exploring.

libcxx/test/std/ranges/range.adaptors/range.common.view/ctor.view.pass.cpp
54 ↗(On Diff #360189)

Same comment as above with std::is_constant_evaluated().

This revision now requires changes to proceed.Jul 20 2021, 12:44 PM
zoecarver updated this revision to Diff 360989.Jul 22 2021, 2:57 PM

Apply review comments.

ldionne accepted this revision.Jul 23 2021, 8:32 AM
This revision is now accepted and ready to land.Jul 23 2021, 8:32 AM
This revision was automatically updated to reflect the committed changes.