This will unblock work on ranges::view. Based on D101396.
Details
- Reviewers
 ldionne cjdb EricWF • Quuxplusone - Group Reviewers
 Restricted Project - Commits
 - rG5671ff20d92b: [libcxx] Implement view.interface.
 
Diff Detail
- Repository
 - rG LLVM Github Monorepo
 
Unit Tests
Event Timeline
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.  | |
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?  | |
| libcxx/include/__ranges/data.h | ||
|---|---|---|
| 58 ↗ | (On Diff #342305) | missing !?  | 
| 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.  | |
| 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.  | |
| libcxx/include/__ranges/view_interface.h | ||
|---|---|---|
| 118 | Remove all the nodiscards except for empty.  | |
| libcxx/include/__ranges/view_interface.h | ||
|---|---|---|
| 118 | It would be appropriate to mark them _LIBCPP_NODISCARD_EXT (with tests).  | |
| libcxx/include/__ranges/view_interface.h | ||
|---|---|---|
| 118 | 
 Please elaborate. 
 Why is this different to [[nodiscard]] + disabling the warning?  | |
| libcxx/include/__ranges/view_interface.h | ||
|---|---|---|
| 118 | 
 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()).  | |
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?  | |
| 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.  | |
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!  | |
Should be sorted!