[libcxx-commits] [PATCH] D110370: [libc++][NFCI] Simplify std::ranges::subrange

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

Quuxplusone requested changes to this revision.
Quuxplusone added a comment.
This revision now requires changes to proceed.

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



================
Comment at: libcxx/include/__ranges/subrange.h:68
 
   enum class _LIBCPP_ENUM_VIS subrange_kind : bool { unsized, sized };
 
----------------
Commit message: This cannot be both an ABI break //and// "[NFCI]". Pick one or the other. ;)


================
Comment at: libcxx/include/__ranges/subrange.h:81
+    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>;
----------------
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.


================
Comment at: libcxx/include/__ranges/subrange.h:101
       requires (_Kind == subrange_kind::sized)
-      : _Base(_VSTD::move(__iter), __sent, __n)
+      : __begin_(_VSTD::move(__iter)), __end_(__sent), __size_(__n)
     {
----------------
If you're moving `__iter`, you might as well move `__sent` as well.


================
Comment at: libcxx/include/__ranges/subrange.h:169
       else
-        return __to_unsigned_like(this->__end_ - this->__begin_);
+        return __to_unsigned_like(__end_ - __begin_);
     }
----------------
`_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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110370