Page MenuHomePhabricator

[libcxx] Implement view.interface.
ClosedPublic

Authored by zoecarver on May 2 2021, 8:37 PM.

Details

Reviewers
ldionne
cjdb
EricWF
Quuxplusone
Group Reviewers
Restricted Project
Commits
rG5671ff20d92b: [libcxx] Implement view.interface.
Summary

This will unblock work on ranges::view. Based on D101396.

Refs http://eel.is/c++draft/view.interface.

Diff Detail

Event Timeline

zoecarver created this revision.May 2 2021, 8:37 PM
zoecarver requested review of this revision.May 2 2021, 8:37 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 2 2021, 8:37 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
zoecarver planned changes to this revision.May 2 2021, 8:41 PM

I wanted to get this review up before Monday so that other folks can start on their views work. I'm planning a few changes (mainly to add some tests) so this isn't quite ready for review yet.

libcxx/include/__ranges/view_interface.h
38

I will remove this once the patch lands and I rebase.

131

Add a message here.

150

Review note: this is used until ranges::prev is implemented.

zoecarver updated this revision to Diff 342305.May 2 2021, 8:41 PM
  • Fix spacing.
cjdb requested changes to this revision.May 2 2021, 9:30 PM
cjdb added a subscriber: tcanens.

A good start! I'd appreciate some libcxx tests confirming that _LIBCPP_ASSERT fires when a view is empty, and some noexcept tests.

libcxx/include/__ranges/view_interface.h
38

It doesn't look like the concept is used in your patch anyway?

46

Please make this a reference.

60

Oh, this issue. I remember when gcc -fconcepts had this problem back in the day :(

150

Thanks for highlighting this, was about to comment.

libcxx/test/std/ranges/range.utility/view.interface/view.interface.pass.cpp
35

Can we please get a pointer-to-member and one pointer-to-member function too?

49–52

Nit: consider making this a hidden friend.

103

Please either rename or document why this function exists.

112

Nit: fv or fr? (I'd personally go with single-letter names to avoid the mental "what does this letter correspond to?".)

156

I think view_interface::data (the real one) is a function template, so it'll prefer the existing member function. @tcanens how far from the mark am I?

This revision now requires changes to proceed.May 2 2021, 9:30 PM
tcanens added inline comments.May 2 2021, 9:41 PM
libcxx/include/__ranges/data.h
58 ↗(On Diff #342305)

missing !?

zoecarver added inline comments.May 3 2021, 9:44 AM
libcxx/include/__ranges/view_interface.h
46

In this case T = Derived so I don't think it matters. I've added a test where *this is of type move only rvalue.

60

:(

Also, I couldn't do __can_empty inline.

And if you don't have this, it fails in a very confusing way. Spent a lot of time figuring out why view_interface sort of thought _Derived was an incomplete type but could call members on the type. I finally figured it out when I discovered that it only appeared to be an incomplete type when evaluating the constraints. I suspect clang is simply forgetting a record = record->getDefinition() somewhere.

libcxx/test/std/ranges/range.utility/view.interface/view.interface.pass.cpp
49–52

Am I able to define a hidden friend out of line? I don't want to update forward_iterator (unless we agree that would be a good idea).

156

That would make sense, except that it's the child that's getting called, i.e, view_interface::data. Regardless of which one is a template, I thought it would always go for the parent's definition.

zoecarver updated this revision to Diff 342427.May 3 2021, 9:45 AM
  • Apply review comments and general cleanup.
cjdb added inline comments.May 3 2021, 12:48 PM
libcxx/test/std/ranges/range.utility/view.interface/view.interface.pass.cpp
49–52

Oh, I see, it's not for the above type. My mistake.

156

BTW, the same is true for SFINAE.

zoecarver planned changes to this revision.May 6 2021, 5:46 PM
zoecarver added inline comments.
libcxx/include/__ranges/view_interface.h
118

Remove all the nodiscards except for empty.

Quuxplusone added inline comments.May 6 2021, 6:14 PM
libcxx/include/__ranges/view_interface.h
118

It would be appropriate to mark them _LIBCPP_NODISCARD_EXT (with tests).

cjdb added inline comments.May 6 2021, 9:43 PM
libcxx/include/__ranges/view_interface.h
118

Remove all the nodiscards except for empty.

Please elaborate.

It would be appropriate to mark them _LIBCPP_NODISCARD_EXT (with tests).

Why is this different to [[nodiscard]] + disabling the warning?

zoecarver added inline comments.May 11 2021, 11:08 AM
libcxx/include/__ranges/view_interface.h
118

Please elaborate.

We've discussed elsewhere, I think libc++ is only going to apply nodiscard to methods where a user might think that a side effect is happening (such as .empty()).

  • Apply review comments.
zoecarver updated this revision to Diff 347804.Tue, May 25, 3:25 PM

Rebase on main.

ldionne requested changes to this revision.Mon, May 31, 2:46 PM

Generally LGTM, with some comments!

libcxx/include/CMakeLists.txt
52–54

Should be sorted!

libcxx/include/__ranges/view_interface.h
39

Can drop nodiscard.

56

Why is that necessary? Is that the workaround for gcc -fconcepts you're discussing in the comments below?

150

This can be changed now (and also in the noexcept(...) above).

libcxx/test/std/ranges/range.utility/view.interface/view.interface.pass.cpp
13

I don't think this is necessary anymore.

185

Can you elaborate on what this TODO means?

This revision now requires changes to proceed.Mon, May 31, 2:46 PM
CaseyCarter added inline comments.Tue, Jun 1, 6:56 AM
libcxx/include/__ranges/view_interface.h
50

You're missing that the implicit conversion to of this expression to bool could be potentially throwing. We use something like:

c++
template <class T>
void implicit_convert_to(type_identity_t<T>) noexcept;
/* ... */
  noexcept(noexcept(implicit_convert_to<bool>(ranges::begin(__derived()) == ranges::end(__derived()))))
56

This is necessary to workaround https://bugs.llvm.org/show_bug.cgi?id=44833.

zoecarver marked 8 inline comments as done.Tue, Jun 1, 11:00 AM
zoecarver added inline comments.
libcxx/include/__ranges/view_interface.h
50

Done and added a regression test.

libcxx/test/std/ranges/range.utility/view.interface/view.interface.pass.cpp
185

No longer applies, removed.

(IIRC, that assert failed because it was using view_interface's implementation for data.)

zoecarver updated this revision to Diff 349025.Tue, Jun 1, 11:01 AM
zoecarver marked 2 inline comments as done.

Apply review comments.

zoecarver updated this revision to Diff 349027.Tue, Jun 1, 11:03 AM
  • Rebase.
  • -- -> prev (again).
ldionne accepted this revision.Tue, Jun 1, 11:39 AM

LGTM, but please wait for CI to finish.

(Also, we just spoke offline and you mentioned you needed to update the Status paper and the synopsis - please do it before landing).

libcxx/include/__ranges/view_interface.h
56

Thanks!

This revision was not accepted when it landed; it landed in state Needs Review.Tue, Jun 1, 12:35 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.