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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante accepted this revision as: Mordante.
Mordante added a comment.

In D112631#3090943 <https://reviews.llvm.org/D112631#3090943>, @jloser wrote:

> In D112631#3090909 <https://reviews.llvm.org/D112631#3090909>, @cjdb wrote:
>
>> In D112631#3090888 <https://reviews.llvm.org/D112631#3090888>, @jloser wrote:
>>
>>> 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.
>>
>> Ah, yes, I forgot about the completeness requirement, although I'm not really concerned about that one. All of our requirements hinge on completeness.
>>
>>> 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?
>>
>> We can't do a static assert because the special member functions are allowed to exist. What I'm proposing is:
>>
>>   constexpr bool empty() requires forward_­range<D> and view<D> and derived_from<D, view_interface<D>>;
>
> I see. Yeah, we can (and still be conforming AFAIK) add the additional requirements of `view<D>` and `derived_from<D, view_interface<D>>` to be checked on every member function of `view_interface` . This does have additional compile-time costs as you know, but does ensure a bit of safety. I'm generally in favor of this kind of tradeoff. What's the status quo with other types (ranges or others) with regard to these requirements on classes? Right now, it's IFNDR which isn't great from a user perspective. Thoughts, @ldionne @Quuxplusone?

Personally I would prefer to stick to the wording of the Standard. Since all these functions call the exposition only function `derived`, how about adding these additional requirements to these two functions?

IMO the proposed changes to the requirements should be done in a separate patch. For this one LGTM!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112631