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
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!