ldionne cjdb EricWF • Quuxplusone
- Group Reviewers
- rG5671ff20d92b: [libcxx] Implement view.interface.
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.
I will remove this once the patch lands and I rebase.
Add a message here.
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.
It doesn't look like the concept is used in your patch anyway?
Please make this a reference.
Oh, this issue. I remember when gcc -fconcepts had this problem back in the day :(
Thanks for highlighting this, was about to comment.
Can we please get a pointer-to-member and one pointer-to-member function too?
Nit: consider making this a hidden friend.
Please either rename or document why this function exists.
Nit: fv or fr? (I'd personally go with single-letter names to avoid the mental "what does this letter correspond to?".)
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?
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.
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.
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).
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.
Why is this different to [[nodiscard]] + disabling the warning?
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!
Should be sorted!
Can drop nodiscard.
Why is that necessary? Is that the workaround for gcc -fconcepts you're discussing in the comments below?
This can be changed now (and also in the noexcept(...) above).
I don't think this is necessary anymore.
Can you elaborate on what this TODO means?
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()))))
This is necessary to workaround https://bugs.llvm.org/show_bug.cgi?id=44833.