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

[Joe Loser via Phabricator via libcxx-commits]

jloser added inline comments.


================
Comment at: libcxx/include/span:179-183
+            typename enable_if<!__is_std_span<remove_cvref_t<_Tp>>::value, nullptr_t>::type,
         // is not a specialization of array
-            typename enable_if<!__is_std_array<_Tp>::value, nullptr_t>::type,
+            typename enable_if<!__is_std_array<remove_cvref_t<_Tp>>::value, nullptr_t>::type,
         // is_array_v<Container> is false,
             typename enable_if<!is_array_v<_Tp>, nullptr_t>::type,
----------------
Quuxplusone wrote:
> Quuxplusone wrote:
> > If you're updating these, might as well make them all use e.g.
> > `enable_if_t<!__is_std_span<remove_cvref_t<_Tp>>::value>,`
> Actually, isn't this type-trait completely unused now? Let's remove lines 172–194!
> 
> If I'm wrong and it //is// used somewhere, then please add a test for
> ```
> struct Widget { friend int size(Widget*); };
> static_assert(!std::is_constructible_v<std::span<Widget>, Widget(&)[]>);
> ```
> and remove the `, nullptr_t` on lines 179 and 181.
It's unused and removed now. Good call!


================
Comment at: libcxx/include/span:70
+    template<class R>
+      constexpr explicit(extent != dynamic_extent) span(R&& r);
     constexpr span(const span& other) noexcept = default;
----------------
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.


================
Comment at: libcxx/test/std/containers/views/span.cons/iterator_len.verify.cpp:28
+  int arr[] = {1, 2, 3};
+  createImplicitSpan<int, 1>(arr, 1);
+
----------------
Quuxplusone wrote:
> If you decide to keep a `.verify.cpp` test for `span`, then please add a test for
> ```
> std::span<const int> sp = {0, 0};
> ```
> Today we accept that (interpreting `0` as a null pointer constant); after this PR we should correctly reject it (as libstdc++ and MSVC already do).
> 
Added said test and we properly reject it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110503