[libcxx-commits] [PATCH] D112631: [libc++][test] Fix invalid test for views::view_interface

[Joe Loser via Phabricator via libcxx-commits]

jloser added a comment.

In D112631#3090641 <https://reviews.llvm.org/D112631#3090641>, @cjdb wrote:

> Nice catch, thanks.
>
> I think a conforming implementation is allowed to put those two requirements on each of the member functions: WDYT about adding that in this patch?

Do you mind elaborating on your suggestion here? In `view.interface.general` note 2 states: that "`D shall be complete, and model both derived_­from<view_­interface<D>> and view`".  So, I'm assuming you're referring to the completeness and `view` requirements?  Note the requirement of completeness is dependent on where it's checked because `D` can be an incomplete type as it mentions in the note.

I think an improvement in the QoI that may be a nice middle-ground is to put a `static_assert(view<_Derived>, "Derived class must be a view.");` in `view_interface`. However, this breaks several ranges tests - specifically, `subrange` which inherits from `view_interface`. I haven't looked too closely, but at the time of the inheritance, `subrange` wouldn't be a complete type, so we may have a bug somewhere with incompleteness of types and some view/range concepts somewhere. I'd recommend we investigate that in a follow-up. What do you think?



================
Comment at: libcxx/test/std/ranges/range.utility/view.interface/view.interface.pass.cpp:105
   constexpr RAIter begin() const { return RAIter(const_cast<int*>(buff)); }
   constexpr RAIter end() const { return RAIter(const_cast<int*>(buff) + 8); }
 };
----------------
cjdb wrote:
> I don't mind if it's done in this patch or a second one, but could you please strip the const_casts? A const-qualified range shouldn't be returning a mutable iterator.
Good spot. I'm happy to do that in a separate patch as the `const_cast`s are present in a few of the range test types here and it's orthogonal to this particular test fix. :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112631/new/

https://reviews.llvm.org/D112631