Page MenuHomePhabricator

[libcxx][ranges] Implement views::all.
ClosedPublic

Authored by zoecarver on May 6 2021, 3:55 PM.

Details

Reviewers
cjdb
ldionne
Quuxplusone
EricWF
Group Reviewers
Restricted Project
Commits
rGc820b494d6e1: [libcxx][ranges] Implement views::all.

Diff Detail

Event Timeline

zoecarver created this revision.May 6 2021, 3:55 PM
zoecarver requested review of this revision.May 6 2021, 3:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 6 2021, 3:55 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript

I think I need to write some more tests for this.

libcxx/include/__ranges/view_interface.h
41 ↗(On Diff #343519)

Sorry about these random changes. I need to rebase the whole stack at some point (at which point these will disappear).

tcanens added a subscriber: tcanens.May 7 2021, 3:42 PM
tcanens added inline comments.
libcxx/include/__ranges/views_all.h
38 ↗(On Diff #343519)

This should be forward.

  • Apply review comment.
  • Rebase.
Quuxplusone requested changes to this revision.May 11 2021, 2:26 PM
Quuxplusone added a subscriber: BRevzin.
Quuxplusone added inline comments.
libcxx/include/__ranges/views_all.h
1 ↗(On Diff #344536)

For the name of this header, I think you can get away with <__ranges/all.h>, since the name of the entity is all (not views_all). Or is this going to pose some kind of problem when we get to some other specific view whose name is inappropriate and/or already taken?

13–17 ↗(On Diff #344536)

alphabetize (also in view_interface.h)

52 ↗(On Diff #344536)

This may be a complete red herring: I vaguely recall an LWG issue and/or paper that pointed out that a whole bunch of brace-initializations in the Standard needed to be parens-initializations, and/or needed to stop using CTAD, because they would do the wrong thing under CTAD. All I can actually find by googling/gmail-searching, though, is @BRevzin's https://cplusplus.github.io/LWG/issue3474 . Does anyone know whether (1) such a blanket issue exists, (2) should exist, and/or (3) applies here? @tcanens, thoughts?

(This code looks correct-w.r.t.-the-standard-as-written, modulo noexcept.)

58 ↗(On Diff #344536)

What's the point of const here? (I think "none, remove it," but I could be missing something subtle.)

61 ↗(On Diff #344536)

} // namespace views

38 ↗(On Diff #343519)

Here and throughout, [my understanding is that] "expression-equivalent" requires an appropriate noexcept clause. (Please check your other PRs as well.)

libcxx/test/std/ranges/range.adaptors/range.all.pass.cpp
22–23

stdr, and presumably stdv
(My personal preference is rg and rv, but stdr is already in the test suite.)
However, bonus points if you can just not use namespace aliases. It seems like you're already avoiding them in some cases, e.g. std::ranges::view_base on line 26. Can you just do that throughout? Makes it a lot easier to grep for test coverage, because git grep std::views::all libcxx/test/ will actually return some hits.

86

You need tests that hit every different overload in the dispatch, and also significant CTAD cases. At least, test that views::all(x) has the same type as x when x is a subrange, and when x is a ref_view.
For the tests you've got now, I think it'd be better to do something that differentiates a copy-constructed Range from a default-constructed Range. For example, give Range a constructor constexpr explicit Range(int *b, int *e) : b_(b), e_(e) {}, then construct `

int i[2];
auto range = Range(i, i+2);
auto v = std::views::all(range);
ASSERT_SAME_TYPE(decltype(v), std::ranges::ref_view<Range>);
assert(std::ranges::begin(v) == i);
assert(std::ranges::end(v) == i+2);
Test that `views::all(view_lvalue)` works just as well as `views::all(View())`.
Test that `views::all(Range())` SFINAEs away (I assume).
Test that `const auto range = Range(i, i+2); std::views::all(range)` gives you a `std::ranges::ref_view<const Range>` (I assume).
Maybe test with a range that is not a common_range (i.e., one with a sentinel type different from its iterator type), and that that gives you back a `subrange` with the appropriate template parameters.
This revision now requires changes to proceed.May 11 2021, 2:26 PM
cjdb added inline comments.May 11 2021, 4:42 PM
libcxx/include/__ranges/views_all.h
52 ↗(On Diff #344536)

Yes, I think this is informally called Tim-initialisation-fiasco in LWG since he pointed it out. I distinctly remember something relevant in Cologne or Belfast, but I thought it was all resolved there. I've gotten into the habit of putting // braces intentional on anything that genuinely needs brace-init in template code.

38 ↗(On Diff #343519)

Here and throughout, [my understanding is that] "expression-equivalent" requires an appropriate noexcept clause. (Please check your other PRs as well.)

Yep.

tcanens added inline comments.May 11 2021, 5:00 PM
libcxx/include/__ranges/views_all.h
51 ↗(On Diff #344536)

This needs to be SFINAE friendly if the subrange construction is ill-formed.

52 ↗(On Diff #344536)

I vaguely recall an LWG issue and/or paper that pointed out that a whole bunch of brace-initializations in the Standard needed to be parens-initializations, and/or needed to stop using CTAD, because they would do the wrong thing under CTAD.

I think you are thinking about LWG3524 which will be resolved by P2367R0. The other braces in the ranges clause - including these - have no semantic effect and may be replaced with parens at editorial discretion: https://github.com/cplusplus/draft/issues/4593.

Yes, I think this is informally called Tim-initialisation-fiasco in LWG since he pointed it out. I distinctly remember something relevant in Cologne or Belfast, but I thought it was all resolved there.

That's a different one where the concept currently named default_initializable did not require T{} to work until LWG3149 was moved in Belfast, and so all the default member initializers needed to be T t = T(); instead of T t{};

Quuxplusone added inline comments.May 11 2021, 5:04 PM
libcxx/include/__ranges/views_all.h
52 ↗(On Diff #344536)

I've gotten into the habit of putting // braces intentional on anything that genuinely needs brace-init in template code.

I almost approve of that idea! (The problem I see with it is that I think the kind of person who'd put braces when they weren't needed is also the kind of person who'd put // braces intentional out of a misplaced sense of purpose. ;)) In a perfect world, it'd be useful to have a documented list of "reasons libc++ might use braces," and then the comment could be, like // braces for reason #2 on that list.
Here we have a double whammy: "braces intentional(?)" and "CTAD intentional(?)".

I would approve even more of a // CTAD intentional comment — which wouldn't really need to explain "why," because CTAD does like three things at once and the reason is always "because we want all those things to happen, I guess." Which is also why I'm even more suspicious of unintended CTAD.
https://quuxplusone.github.io/blog/2018/12/09/wctad/

@cjdb, for my own curiosity: Do you think it's feasible for a library to implement C++20 Ranges without ever using CTAD in its implementation? Like, could one realistically spell out the intended template arguments to subrange here? or is the CTAD hiding some really hairy and irreducible complexity? (Leave aside for the moment whether you think it'd be a good idea to remove CTAD from libc++'s implementation. Actually my kneejerk assumption is that it would not be pragmatic to remove, especially at this early stage, simply because the Standard gives reference implementations in terms of CTAD and so removing our CTAD would simply increase our chances of getting behavior subtly different from the Standard. But in a hypothetical Other Vendor's Implementation, would removing CTAD here and throughout be physically feasible?)

Quuxplusone added inline comments.May 11 2021, 5:10 PM
libcxx/include/__ranges/views_all.h
52 ↗(On Diff #344536)

"I think you are thinking about LWG3524 which will be resolved by P2367R0." — Ah. Yes. (The issue there, IIUC, is that foo{a, b} implies left-to-right evaluation of the side effects of a and b; an unsequenced function invocation like ranges::foo(a, b) can never be made expression-equivalent to it. So LWG3524 doesn't apply to foo{} or foo{a}, only to things with multiple arguments.)

zoecarver updated this revision to Diff 344611.May 11 2021, 5:34 PM

Apply Arthur's comments.

zoecarver marked 8 inline comments as done.May 11 2021, 5:37 PM

@Quuxplusone I applied all your comments expect for the one about the test. I'll apply that one tomorrow, when I have the time :)

libcxx/test/std/ranges/range.adaptors/range.all.pass.cpp
23

Remove these now that they're no longer used.

zoecarver added inline comments.May 11 2021, 5:41 PM
libcxx/include/__ranges/views_all.h
52 ↗(On Diff #344536)

@cjdb, for my own curiosity: Do you think it's feasible for a library to implement C++20 Ranges without ever using CTAD in its implementation? Like, could one realistically spell out the intended template arguments to subrange here? or is the CTAD hiding some really hairy and irreducible complexity? (Leave aside for the moment whether you think it'd be a good idea to remove CTAD from libc++'s implementation. Actually my kneejerk assumption is that it would not be pragmatic to remove, especially at this early stage, simply because the Standard gives reference implementations in terms of CTAD and so removing our CTAD would simply increase our chances of getting behavior subtly different from the Standard. But in a hypothetical Other Vendor's Implementation, would removing CTAD here and throughout be physically feasible?)

drop_view, for example, relies on CTAD like this:

template<class R>
    drop_view(R&&, range_difference_t<R>) -> drop_view<views::all_t<R>>;

So, I think the answer is, unfortunately, "no."

zoecarver updated this revision to Diff 344957.May 12 2021, 2:09 PM

Add suggested tests.

ldionne requested changes to this revision.Thu, Jun 3, 1:22 PM

Implementation LGTM, seems like we can improve tests to some degree (my comment but also Arthur's comment which doesn't appear to have been addressed yet).

libcxx/test/std/ranges/range.adaptors/range.all.pass.cpp
86

We seem to be missing tests for noexcept-ness.

This revision now requires changes to proceed.Thu, Jun 3, 1:22 PM
zoecarver updated this revision to Diff 349970.Fri, Jun 4, 2:23 PM

Add noexcept tests.

zoecarver marked 2 inline comments as done.Fri, Jun 4, 2:24 PM
ldionne accepted this revision.Mon, Jun 7, 1:34 PM

I'd like to see CI passing, but still LGTM. @Quuxplusone are you still red on this?

@Quuxplusone friendly ping. I'm going to rebase this shortly. After the tests pass I'm planning to land it.

Quuxplusone accepted this revision.Fri, Jun 11, 10:22 AM

Sure, I don't need to be red anymore.

libcxx/test/std/ranges/range.adaptors/range.all.pass.cpp
24

Please remove the default template parameter here: just template<bool IsNoexcept>. Otherwise, you run the risk of writing ViewImpl (CTAD) when you meant ViewImpl<false>. Be explicit.

38

Let's static_assert here on both specializations, unless there's a reason not to.

static_assert(std::ranges::view<ViewImpl<true>>);
static_assert(std::ranges::view<ViewImpl<false>>);

In fact, you might consider renaming ViewImpl to just View, merely to shorten the line lengths throughout. Ditto for CopyableViewImpl.

95–96

Not a blocker, but I will note for @ldionne's benefit that we have recently seen (in D103769) that merely testing top-level noexceptness is not sufficient to catch implementation bugs. What we actually want to test here is that if the foo operation throws an exception, that exception is correctly propagated all the way up to the top level. It's not sufficient to just test that the top level is not marked noexcept, because the implementor may have accidentally marked one of the intermediate levels noexcept. We need runtime tests to actually verify that the "exception pipe" from low level to top level is unbroken.

(They should not be marking intermediate levels noexcept, because that's often harmful and never helpful. But empirically I observe them doing it anyway.)
https://quuxplusone.github.io/blog/2018/06/12/attribute-noexcept-verify/
https://stackoverflow.com/questions/66459162/where-do-standard-library-or-compilers-leverage-noexcept-move-semantics-other-t

This revision is now accepted and ready to land.Fri, Jun 11, 10:22 AM

Rebase. This is just to get the CI going, I'll apply your comments shortly, Arthur.

zoecarver updated this revision to Diff 351850.Mon, Jun 14, 6:16 AM
  • Rebase and fix header includes.
  • Remove default template param.
  • ViewImpl -> View.
This revision was landed with ongoing or failed builds.Mon, Jun 14, 7:41 AM
This revision was automatically updated to reflect the committed changes.