[libcxx-commits] [PATCH] D110503: [libc++] Implement P1394r4 for span: range constructor

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

Quuxplusone added inline comments.


================
Comment at: libcxx/include/span:232
+      (void)__last;
+      _LIBCPP_ASSERT(__first <= __last, "invalid range in span's constructor (iterator, sentinel)");
+      _LIBCPP_ASSERT(__last - __first == _Extent,
----------------
This has the same problem as in the `string_view` PR: I don't think `operator<=` is always valid here. Please add a test for an iterator/sentinel pair that doesn't support `<=`.

The issue is not that `__first <= __last` might be //false//; the issue is that it might not be well-formed at all.


================
Comment at: libcxx/include/span:70
+    template<class R>
+      constexpr explicit(extent != dynamic_extent) span(R&& r);
     constexpr span(const span& other) noexcept = default;
----------------
jloser wrote:
> Quuxplusone wrote:
> > `s/extent/Extent/`
> For consistency with our synopsis, I'll use `Extent`. But do note the standard uses `extent` in the synopsis. I don't like differing just for preference with the implementation of how we use `Extent` everywhere, but I like being locally consistent within a neighborhood of code.
Ah, yeah, I noticed the Standard's use of `extent` yesterday while working on https://quuxplusone.github.io/blog/2021/10/03/p2447-span-from-initializer-list/ . So it wasn't //just// a typo. I agree, I like consistency. But today I have no strong preference between the spellings: if we want to use `extent` consistently, or `Extent` consistently, I don't care.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110503