[libcxx-commits] [PATCH] D102006: [libcxx][ranges] Add range.subrange.

[Mark de Wever via Phabricator via libcxx-commits]

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

Please validate the visibility for all types
template classes -> `_LIBCPP_TEMPLATE_VIS`
enums -> `_LIBCPP_ENUM_VIS`
inline function -> `_LIBCPP_INLINE_VISIBILITY`
For now I only looked at the `subrange` header. Will look at the test at another time.



================
Comment at: libcxx/include/__ranges/subrange.h:40
+  template<class _Range>
+  concept sized_range = range<_Range> && requires(_Range& t) {
+    ranges::size(t);
----------------
Please __uglify t.
Minor nit, since you use `_Range` instead of `_Tp`, how do you feel about using `__r` instead of `__t`?


================
Comment at: libcxx/include/__ranges/subrange.h:50
+      is_pointer_v<decay_t<_To>> &&
+      !same_as<remove_pointer_t<decay_t<_From>>, remove_pointer_t<decay_t<_To>>>);
+
----------------
The Standard uses `not-same-as` instead of `!same_as`.
```
template<class T, class U>
  concept not-same-as =                         // exposition only
    !same_­as<remove_cvref_t<T>, remove_cvref_t<U>>;
```
Is there a reason to deviate?


================
Comment at: libcxx/include/__ranges/subrange.h:63
+
+  template<class _Pair, class _Iter, class _Sent>
+  concept __pair_like_convertible_from =
----------------
Just curious, why these names instead of `_Tp`, `_Up`, and `_Vp`?


================
Comment at: libcxx/include/__ranges/subrange.h:70
+
+  enum class subrange_kind { unsized, sized };
+
----------------
Why not using a bool as underlying type? The Standard uses `enum class subrange_kind : bool { unsized, sized };` .


================
Comment at: libcxx/include/__ranges/subrange.h:119
+    template<class _Range>
+      requires (!same_as<subrange, _Range>) &&
+               borrowed_range<_Range> &&
----------------
Another case of `!same_as` vs `not-same-as`. When using `__not_same_as` this concept can be used instead of `class` like in the Standard.
Also look at the other `!same_as` in this class.


================
Comment at: libcxx/include/__ranges/subrange.h:159
+
+    [[nodiscard]] constexpr bool empty() const { return __begin == __end; }
+
----------------
Please use `NODISCARD_EXT`.


================
Comment at: libcxx/include/__ranges/subrange.h:161
+
+    constexpr make_unsigned_t<iter_difference_t<_Iter>> size() const
+      requires (_Kind == subrange_kind::sized)
----------------
Can you rename this to `__make_unsigned_t`?


================
Comment at: libcxx/include/__ranges/subrange.h:194
+          if constexpr (__store_size)
+            _Base::__size += __to_unsigned_like(-__n);
+          return *this;
----------------
Interesting, hopefully `__n` will never be `std::numeric_limits<iter_difference_t<_Iter>>::min()`


================
Comment at: libcxx/include/type_traits:2311-2314
+template<class _From>
+[[nodiscard]] constexpr auto __to_unsigned_like(_From __x) noexcept {
+  return static_cast<make_unsigned_t<_From>>(__x);
+}
----------------
zoecarver wrote:
> Mordante wrote:
> > cjdb wrote:
> > > Mordante wrote:
> > > > cjdb wrote:
> > > > > I wanna poach this for D101922. WDYT?
> > > > I would like `_LIBCPP_INLINE_VISIBILITY` and I dislike the `_like` suffix. Of course if we remove the `_like` it duplicates `__to_unsigned` defined in `<charconv>`. I start to feel it makes sense to make a header to do some of these signed to unsigned conversions. (I also use them for `<format>` but there I already require `<charconv>`. WDYT? (Note `<charconv>` is available in C++11 as an extension.)
> > > > I wouldn't mind to put up a review to make this change.
> > > This is already in main. I'm not sure why it's showing up in the diff.
> > I see thanks. Any objection to renaming it to `__to_unsigned`? Since `<charconv>` already depends on `<type_traits>` I'll make a patch to let `<charconv>` reuse this function.
> > I see thanks. Any objection to renaming it to __to_unsigned? Since <charconv> already depends on <type_traits> I'll make a patch to let <charconv> reuse this function.
> 
> That would be great, thanks @Mordante.
I've put up D102332.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102006