Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[view.interface] Inconsistencies in the draft #3577

Closed
miscco opened this issue Dec 24, 2019 · 2 comments
Closed

[view.interface] Inconsistencies in the draft #3577

miscco opened this issue Dec 24, 2019 · 2 comments
Labels
lwg Issue must be reviewed by LWG.

Comments

@miscco
Copy link
Contributor

miscco commented Dec 24, 2019 •
edited

In the first paragraph there seem to be some inconsistencies in the current draft.

The data() methods are depicted as follows:

    constexpr auto data() requires contiguous_iterator<iterator_t<D>> {
      return to_address(ranges::begin(derived()));
    }
    constexpr auto data() const
      requires range<const D> && contiguous_iterator<iterator_t<const D>> {
        return to_address(ranges::begin(derived()));
   }

Note that the non-const member function is missing the range<D> requirement

The operator[] member functions are the only ones written in the "old" template style

    template<random_access_range R = D>
      constexpr decltype(auto) operator[](range_difference_t<R> n) {
        return ranges::begin(derived())[n];
      }
    template<random_access_range R = const D>
      constexpr decltype(auto) operator[](range_difference_t<R> n) const {
        return ranges::begin(derived())[n];
      }

They should be written with a requires clause similar to all other methods.

    constexpr decltype(auto) operator[](range_difference_t<D> n) 
      requires random_access_range<D> {
        return ranges::begin(derived())[n];
    }

    constexpr decltype(auto) operator[](range_difference_t<const D> n) const
      requires random_access_range<const D> {
        return ranges::begin(derived())[n];
    }

Finally, the implicit conversion opterator seems to be missing a requirement too.

    constexpr bool empty() requires forward_range<D> {
      return ranges::begin(derived()) == ranges::end(derived());
    }
    constexpr bool empty() const requires forward_range<const D> {
      return ranges::begin(derived()) == ranges::end(derived());
    }

    constexpr explicit operator bool()
      requires requires { ranges::empty(derived()); } {
        return !ranges::empty(derived());
      }
    constexpr explicit operator bool() const
      requires requires { ranges::empty(derived()); } {
        return !ranges::empty(derived());
      }

Note that the conversion operator defers back to ranges::empty(). While I believe that it is implied in ranges::empty() it would improve readability/consistency to require forward_range<D>/forward_range<const D>.
@jensmaurer
Copy link
Member

Unless you can show that a WG21-approved paper was misapplied by the editors, all of your concerns seem to be non-editorial. Note that requires-clauses contribute to partial ordering, which might be detectable once user-supplied overloads end up in an overload set.
@CaseyCarter , could you please forward to LWG?

@jensmaurer jensmaurer added the lwg Issue must be reviewed by LWG. label Dec 25, 2019
@miscco
Copy link
Contributor Author

miscco commented Jan 3, 2020

Life is hard, from conversation with @CaseyCarter :

data() has no requirement on range<Derived> as that is implicit in the requirements of Derived as view<Derived> implies range<Derived>. So this is technically correct, although I would question whether it is good practise to omit the -tautological- requirement only at that single place.

operator() must not be restricted on forward_range as a user might write a custom container that does not fulfill the forward_range requirement. Moreover, if I understand CRTP correctly even for the forward_range case view_interface::empty() would be hidden if there is a Derived::empty() and correctly selected by ranges::empty()

operator[] is worse. As Derived might be an incomplete type range_difference_t<Derived> might not compile. By using the indirection template<random_access_range R = Derived> the operator[] is only ever synthesized once it is used and Derived is complete.

@miscco miscco closed this as completed Jan 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
lwg Issue must be reviewed by LWG.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@miscco @jensmaurer