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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone accepted this revision.
Quuxplusone added a comment.
This revision is now accepted and ready to land.

IIUC, Chris proposes changing `view_interface<D>::empty` from

  template<class _D2 = _Derived>
  [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr bool empty()
    noexcept(noexcept(__implicitly_convert_to<bool>(ranges::begin(__derived()) == ranges::end(__derived()))))
    requires forward_range<_D2>
  {
    return ranges::begin(__derived()) == ranges::end(__derived());
  }

to

  template<class _D2 = _Derived>
  [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr bool empty()
    noexcept(noexcept(__implicitly_convert_to<bool>(ranges::begin(__derived()) == ranges::end(__derived()))))
    requires forward_range<_D2> && view<_D2> && derived_from<_D2, view_interface>
  {
    return ranges::begin(__derived()) == ranges::end(__derived());
  }

This doesn't seem useful to me. It diverges from the Standard wording, slows down compilation, suffers from the "more code = more chances for bugs" problem, and also seems like it might hide user-code bugs (by having these members simply //SFINAE away// in the library-UB case).
I don't particularly object to a `static_assert` like

  template<class _D2 = _Derived>
  [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr bool empty()
    noexcept(noexcept(__implicitly_convert_to<bool>(ranges::begin(__derived()) == ranges::end(__derived()))))
    requires forward_range<_D2>
  {
    static_assert(view<_D2> && derived_from<_D2, view_interface>);
    return ranges::begin(__derived()) == ranges::end(__derived());
  }

but even that suffers from the middle two disadvantages above (slows down compilation, suffers from the "more code = more chances for bugs" problem). So I'm (as usual) in favor of the status quo.



================
Comment at: libcxx/test/std/ranges/range.utility/view.interface/view.interface.pass.cpp:75
   int buff[8] = {0, 1, 2, 3, 4, 5, 6, 7};
   MoveOnlyForwardRange(MoveOnlyForwardRange const&) = delete;
   MoveOnlyForwardRange(MoveOnlyForwardRange &&) = default;
----------------
Consider eliminating line 75, as long as you're here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112631