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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision as: ldionne.
ldionne added a comment.

I'm relying on Arthur's in-depth review of the tests. Ship it once my comments have been resolved, everybody else is happy and CI is passing. Thanks a lot!



================
Comment at: libcxx/include/span:195
 
+#if !defined(_LIBCPP_HAS_NO_RANGES)
+template <class _Range, class _ElementType>
----------------
jloser wrote:
> ldionne wrote:
> > This should be
> > 
> > ```
> > #if _LIBCPP_STD_VER > 20 && !defined(_LIBCPP_HAS_NO_RANGES)
> > ```
> > 
> > That way, once we drop support for compilers that don't implement concepts, we'll just get rid of ` && !defined(_LIBCPP_HAS_NO_RANGES)` entirely.
> I changed it to that, but keep in mind p1394 is C++20 and not C++23. So, what do you think about
> 
> ```
> #if defined(__cpp_lib_concepts) && !defined(_LIBCPP_HAS_NO_RANGES)
> ```
> 
> instead? This will ensure concepts supports and >= C++20.
I had made my comment thinking this was a C++23 feature, but what you have right now (just `#if !defined(_LIBCPP_HAS_NO_RANGES)`) seems like the correct thing to me.


================
Comment at: libcxx/include/span:239
+                          nullptr_t> = nullptr>
+    _LIBCPP_INLINE_VISIBILITY constexpr explicit span(_It __first, size_type __count)
+        : __data{_VSTD::to_address(__first)} {
----------------
Here and elsewhere.


================
Comment at: libcxx/test/std/containers/views/span.cons/iterator_len.fail.cpp:1
+//===------------------------------ span ---------------------------------===//
+//
----------------
Here and throughout.


================
Comment at: libcxx/test/std/containers/views/span.cons/iterator_len.fail.cpp:4
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
----------------
Let's also rename this test to `iterator_len.verify.cpp`, since it's a verify-style test.

Note that we support verify-style failures in `.fail.cpp` tests for historical reasons only and we should move away from doing that, hence my request.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110503