[libc++] Simplify std::ranges::subrange

[ldionne]

Summary

Instead of using a base class to store the members and the optional
size, use [[no_unique_address]] to achieve the same thing without
needing a base class.

Also, as a fly-by:

• Change subrange from struct to class (per the standard)
• Improve the diagnostic for when one doesn't provide a size to the ctor of a sized subrange
• Replace this->member by just member since it's not in a dependent base anymore

This change would be an ABI break due to [[no_unique_address]], but we
haven't shipped ranges anywhere yet, so this shouldn't affect anyone.

[• Quuxplusone]

LbasicallyGTM (and a nice improvement! down with base classes!) mod comments. My trivially-default-constructible comment is significant.

libcxx/include/__ranges/subrange.h

68

Commit message: This cannot be both an ABI break and "[NFCI]". Pick one or the other. ;)

81

This type is not trivially default-constructible. I think you should give it a default ctor _Empty() = default; as well, and then

[[no_unique_address]] _Size __size_ = _Size();

and then your non-sized subrange type will be trivially default-constructible whenever the iterator+sentinel types are. And I think there should be a libcxx/ test for that, because it seems both useful and fragile.

101

If you're moving __iter, you might as well move __sent as well.

169

_VSTD::__to_unsigned_like? Yes; cf. line 210.
Notice that __end_ - __begin_ is sentinel-minus-iterator; I don't know if that's required to be literally iter_difference_t<_Iter>, but I also don't know what else it could be.

[ldionne]

Address review comments

[ldionne]

libcxx/include/__ranges/subrange.h

81

I was going to say "awesome, let's do it and add a test for it". Then I tried, and noticed that std::ranges::subrange<int*> still wasn't trivially default constructible, since we have default member initializers per the spec (http://eel.is/c++draft/range.subrange.general).

So I'm not sure we can actually make subrange trivially default constructible, unless I am missing something.

[• Quuxplusone]

libcxx/include/__ranges/subrange.h

81

Well, subrange<int*> won't be trivially default constructible, but I suppose if you had some iterator type It such that value-initializing It were trivial, then it could be... hmm. Does having a NSDMI actually prevent the class from being trivially default constructible?!
https://godbolt.org/z/1YEWvj71f
Gross. Okay, well, I still like my _Empty() = default idea if it lets you get rid of the icky _Empty(auto) ctor template, but otherwise I guess I have no strong preference either way. I agree it's not going to help with triviality.

[ldionne]

libcxx/include/__ranges/subrange.h

81

Does having a NSDMI actually prevent the class from being trivially default constructible?!

Yeah, I was surprised by that too.

Okay, well, I still like my _Empty() = default idea if it lets you get rid of the icky _Empty(auto) ctor template, but otherwise I guess I have no strong preference either way. I agree it's not going to help with triviality.

It doesn't because we have to construct __size_(__n) below.

libcxx/include/__ranges/subrange.h

namespace ranges {
 
   template<class _Pair, class _Iter, class _Sent>
   concept __pair_like_convertible_from =
     !range<_Pair> && __pair_like<_Pair> &&
     constructible_from<_Pair, _Iter, _Sent> &&
     __convertible_to_non_slicing<_Iter, tuple_element_t<0, _Pair>> &&
     convertible_to<_Sent, tuple_element_t<1, _Pair>>;
 
   enum class _LIBCPP_ENUM_VIS subrange_kind : bool { unsized, sized };
 
-  template<class _Iter, class _Sent, bool>
-  struct __subrange_base {
-    static constexpr bool __store_size = false;
-    _Iter __begin_ = _Iter();
-    _Sent __end_ = _Sent();
-
-    _LIBCPP_HIDE_FROM_ABI
-    constexpr __subrange_base() = default;
-
-    _LIBCPP_HIDE_FROM_ABI
-    constexpr __subrange_base(_Iter __iter, _Sent __sent, make_unsigned_t<iter_difference_t<_Iter>> = 0)
-      : __begin_(_VSTD::move(__iter)), __end_(__sent) { }
-  };
-
-  template<class _Iter, class _Sent>
-  struct __subrange_base<_Iter, _Sent, true> {
-    static constexpr bool __store_size = true;
-    _Iter __begin_ = _Iter();
-    _Sent __end_ = _Sent();
-    make_unsigned_t<iter_difference_t<_Iter>> __size_ = 0;
-
-    _LIBCPP_HIDE_FROM_ABI
-    constexpr __subrange_base() = default;
-
-    _LIBCPP_HIDE_FROM_ABI
-    constexpr __subrange_base(_Iter __iter, _Sent __sent, decltype(__size_) __size)
-      : __begin_(_VSTD::move(__iter)), __end_(__sent), __size_(__size) { }
-  };
-
   template<input_or_output_iterator _Iter, sentinel_for<_Iter> _Sent = _Iter,
            subrange_kind _Kind = sized_sentinel_for<_Sent, _Iter>
              ? subrange_kind::sized
              : subrange_kind::unsized>
     requires (_Kind == subrange_kind::sized || !sized_sentinel_for<_Sent, _Iter>)
-  struct _LIBCPP_TEMPLATE_VIS subrange
-    : public view_interface<subrange<_Iter, _Sent, _Kind>>,
-      private __subrange_base<_Iter, _Sent, _Kind == subrange_kind::sized && !sized_sentinel_for<_Sent, _Iter>> {
-
-    using _Base = __subrange_base<_Iter, _Sent, _Kind == subrange_kind::sized && !sized_sentinel_for<_Sent, _Iter>>;
+  class _LIBCPP_TEMPLATE_VIS subrange
+    : public view_interface<subrange<_Iter, _Sent, _Kind>>
+  {
+  private:
+    static constexpr bool _StoreSize = (_Kind == subrange_kind::sized && !sized_sentinel_for<_Sent, _Iter>);
+    static constexpr bool _MustProvideSizeAtConstruction = !_StoreSize; // just to improve compiler diagnostics
+    struct _Empty { constexpr _Empty(auto) noexcept { } };
+    using _Size = conditional_t<_StoreSize, make_unsigned_t<iter_difference_t<_Iter>>, _Empty>;
+    [[no_unique_address]] _Iter __begin_ = _Iter();
+    [[no_unique_address]] _Sent __end_ = _Sent();
+    [[no_unique_address]] _Size __size_ = 0;
 
+  public:
     _LIBCPP_HIDE_FROM_ABI
     subrange() requires default_initializable<_Iter> = default;
 
     _LIBCPP_HIDE_FROM_ABI
     constexpr subrange(__convertible_to_non_slicing<_Iter> auto __iter, _Sent __sent)
-      requires (!_Base::__store_size)
-      : _Base(_VSTD::move(__iter), __sent) {}
+      requires _MustProvideSizeAtConstruction
+      : __begin_(_VSTD::move(__iter)), __end_(std::move(__sent))
+    { }
 
     _LIBCPP_HIDE_FROM_ABI
     constexpr subrange(__convertible_to_non_slicing<_Iter> auto __iter, _Sent __sent,
                        make_unsigned_t<iter_difference_t<_Iter>> __n)
       requires (_Kind == subrange_kind::sized)
-      : _Base(_VSTD::move(__iter), __sent, __n)
+      : __begin_(_VSTD::move(__iter)), __end_(std::move(__sent)), __size_(__n)
     {
       if constexpr (sized_sentinel_for<_Sent, _Iter>)
-        _LIBCPP_ASSERT((this->__end_ - this->__begin_) == static_cast<iter_difference_t<_Iter>>(__n),
+        _LIBCPP_ASSERT((__end_ - __begin_) == static_cast<iter_difference_t<_Iter>>(__n),
           "std::ranges::subrange was passed an invalid size hint");
     }
 
     template<__different_from<subrange> _Range>
       requires borrowed_range<_Range> &&
                __convertible_to_non_slicing<iterator_t<_Range>, _Iter> &&
                convertible_to<sentinel_t<_Range>, _Sent>
     _LIBCPP_HIDE_FROM_ABI
     constexpr subrange(_Range&& __range)
-      requires (!_Base::__store_size)
-      : subrange(ranges::begin(__range), ranges::end(__range)) { }
+      requires (!_StoreSize)
+      : subrange(ranges::begin(__range), ranges::end(__range))
+    { }
 
     template<__different_from<subrange> _Range>
       requires borrowed_range<_Range> &&
                __convertible_to_non_slicing<iterator_t<_Range>, _Iter> &&
                convertible_to<sentinel_t<_Range>, _Sent>
     _LIBCPP_HIDE_FROM_ABI
     constexpr subrange(_Range&& __range)
-      requires _Base::__store_size && sized_range<_Range>
-      : subrange(__range, ranges::size(__range)) { }
-
+      requires _StoreSize && sized_range<_Range>
+      : subrange(__range, ranges::size(__range))
+    { }
 
     template<borrowed_range _Range>
       requires __convertible_to_non_slicing<iterator_t<_Range>, _Iter> &&
                convertible_to<sentinel_t<_Range>, _Sent>
     _LIBCPP_HIDE_FROM_ABI
     constexpr subrange(_Range&& __range, make_unsigned_t<iter_difference_t<_Iter>> __n)
       requires (_Kind == subrange_kind::sized)
-      : subrange(ranges::begin(__range), ranges::end(__range), __n) { }
+      : subrange(ranges::begin(__range), ranges::end(__range), __n)
+    { }
 
     template<__different_from<subrange> _Pair>
       requires __pair_like_convertible_from<_Pair, const _Iter&, const _Sent&>
     _LIBCPP_HIDE_FROM_ABI
-    constexpr operator _Pair() const { return _Pair(this->__begin_, this->__end_); }
+    constexpr operator _Pair() const {
+      return _Pair(__begin_, __end_);
+    }
 
     _LIBCPP_HIDE_FROM_ABI
     constexpr _Iter begin() const requires copyable<_Iter> {
-      return this->__begin_;
+      return __begin_;
     }
 
     [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr _Iter begin() requires (!copyable<_Iter>) {
-      return _VSTD::move(this->__begin_);
+      return _VSTD::move(__begin_);
     }
 
     _LIBCPP_HIDE_FROM_ABI
-    constexpr _Sent end() const { return this->__end_; }
+    constexpr _Sent end() const {
+      return __end_;
+    }
 
-    [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr bool empty() const { return this->__begin_ == this->__end_; }
+    [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr bool empty() const {
+      return __begin_ == __end_;
+    }
 
     _LIBCPP_HIDE_FROM_ABI
     constexpr make_unsigned_t<iter_difference_t<_Iter>> size() const
       requires (_Kind == subrange_kind::sized)
     {
-      if constexpr (_Base::__store_size)
-        return this->__size_;
+      if constexpr (_StoreSize)
+        return __size_;
       else
-        return __to_unsigned_like(this->__end_ - this->__begin_);
+        return _VSTD::__to_unsigned_like(__end_ - __begin_);
     }
 
     [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr subrange next(iter_difference_t<_Iter> __n = 1) const&
-      requires forward_iterator<_Iter> {
+      requires forward_iterator<_Iter>
+    {
       auto __tmp = *this;
       __tmp.advance(__n);
       return __tmp;
     }
 
     [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr subrange next(iter_difference_t<_Iter> __n = 1) && {
       advance(__n);
       return _VSTD::move(*this);
     }
 
     [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr subrange prev(iter_difference_t<_Iter> __n = 1) const
-      requires bidirectional_iterator<_Iter> {
+      requires bidirectional_iterator<_Iter>
+    {
       auto __tmp = *this;
       __tmp.advance(-__n);
       return __tmp;
     }
 
     _LIBCPP_HIDE_FROM_ABI
     constexpr subrange& advance(iter_difference_t<_Iter> __n) {
       if constexpr (bidirectional_iterator<_Iter>) {
         if (__n < 0) {
-          ranges::advance(this->__begin_, __n);
-          if constexpr (_Base::__store_size)
-            this->__size_ += _VSTD::__to_unsigned_like(-__n);
+          ranges::advance(__begin_, __n);
+          if constexpr (_StoreSize)
+            __size_ += _VSTD::__to_unsigned_like(-__n);
           return *this;
         }
       }
 
-      auto __d = __n - ranges::advance(this->__begin_, __n, this->__end_);
-      if constexpr (_Base::__store_size)
-        this->__size_ -= _VSTD::__to_unsigned_like(__d);
+      auto __d = __n - ranges::advance(__begin_, __n, __end_);
+      if constexpr (_StoreSize)
+        __size_ -= _VSTD::__to_unsigned_like(__d);
       return *this;
     }
   };
 
   template<input_or_output_iterator _Iter, sentinel_for<_Iter> _Sent>
   subrange(_Iter, _Sent) -> subrange<_Iter, _Sent>;
 
   template<input_or_output_iterator _Iter, sentinel_for<_Iter> _Sent>

libcxx/test/std/ranges/range.utility/range.subrange/ctor.default.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17
+// UNSUPPORTED: libcpp-no-concepts
+// UNSUPPORTED: libcpp-has-no-incomplete-ranges
+
+// constexpr subrange() requires default_initializable<I>;
+
+#include <ranges>
+
+#include <cassert>
+#include <cstddef>
+
+#include "test_iterators.h"
+
+// An input_or_output_iterator that is not default constructible so we can test
+// the `requires` on subrange's default constructor.
+struct NoDefaultIterator {
+  using difference_type = std::ptrdiff_t;
+  NoDefaultIterator() = delete;
+  NoDefaultIterator& operator++();
+  void operator++(int);
+  int& operator*() const;
+  friend bool operator==(NoDefaultIterator const&, NoDefaultIterator const&);
+};
+static_assert(std::input_or_output_iterator<NoDefaultIterator>);
+
+// A sentinel type for the above iterator
+struct Sentinel {
+  friend bool operator==(NoDefaultIterator const&, Sentinel const&);
+  friend bool operator==(Sentinel const&, NoDefaultIterator const&);
+  friend bool operator!=(NoDefaultIterator const&, Sentinel const&);
+  friend bool operator!=(Sentinel const&, NoDefaultIterator const&);
+};
+
+constexpr bool test() {
+  {
+    static_assert(!std::is_default_constructible_v<std::ranges::subrange<NoDefaultIterator, Sentinel, std::ranges::subrange_kind::sized>>);
+    static_assert(!std::is_default_constructible_v<std::ranges::subrange<NoDefaultIterator, Sentinel, std::ranges::subrange_kind::unsized>>);
+  }
+
+  {
+    using Iter = forward_iterator<int*>;
+    std::ranges::subrange<Iter, Iter, std::ranges::subrange_kind::sized> subrange;
+    assert(subrange.begin() == Iter());
+    assert(subrange.end() == Iter());
+  }
+  {
+    using Iter = forward_iterator<int*>;
+    std::ranges::subrange<Iter, Iter, std::ranges::subrange_kind::unsized> subrange;
+    assert(subrange.begin() == Iter());
+    assert(subrange.end() == Iter());
+  }
+
+  return true;
+}
+
+int main(int, char**) {
+  test();
+  static_assert(test());
+
+  return 0;
+}