[libcxx-commits] [PATCH] D108827: [libc++] Add an assertion in the subrange constructors with a size hint

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

Quuxplusone added inline comments.


================
Comment at: libcxx/test/std/ranges/range.utility/range.subrange/types.h:23
 
 using ForwardSubrange = std::ranges::subrange<ForwardIter, ForwardIter, std::ranges::subrange_kind::unsized>;
 using SizedIntPtrSubrange = std::ranges::subrange<int*, int*, std::ranges::subrange_kind::sized>;
----------------
Huh. `#include <ranges>` from this header, then?


================
Comment at: libcxx/test/std/ranges/range.utility/range.subrange/types.h:55
 
 struct SizedSentinelForwardIter {
     typedef std::forward_iterator_tag            iterator_category;
----------------
Scope creep: It's mildly weird that this type is called `SizedSentinelForwardIterator` when it doesn't actually involve a sentinel type. (Technically, this iterator type is its //own// sentinel type; but I call it mildly weird naming nonetheless.) It would be nicer to name it `SubtractableForwardIterator`.

As we fill out more of the C++20 library, you might also consider adding the sanity-checking static-asserts we couldn't originally do; e.g. on line 91, `static_assert(std::sized_sentinel_for<SubtractableForwardIterator, SubtractableForwardIterator>);`. This kind of assertion is best-practice in real life, to avoid hard-to-debug delayed error messages; I think it's reasonable to also put such assertions in our test-code helper headers. But scope creep, and also not worth fighting for right now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108827