[base] Clean Up make_span functions

This change unifies the various base::make_span(container) utility
functions and simply perfectly forwards the argument to the span
constructor. Two overloads remain, one that deduces the span's
extent automatically and one that let's the caller specify the extent.

In order to do so this change introduces an internal::Extent struct,
which is inspired by std::extent to deduce the right extent from the
passed in argument. This helper is also used to simplify the internal
EnableIfSpanCompatibleArray helper.

Lastly it adds more tests to make sure make_span deduces the right type
and extent.

Bug: 828324

Change-Id: Ib55fc25ab2ad4227daf05d427e0aadf77c671817
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1878367
Commit-Queue: Jan Wilken Dörrie <jdoerrie@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#712325}

base/containers/span.h

@@ -29,6 +29,21 @@ class span;
 
 namespace internal {
 
+template <typename T>
+struct ExtentImpl : std::integral_constant<size_t, dynamic_extent> {};
+
+template <typename T, size_t N>
+struct ExtentImpl<T[N]> : std::integral_constant<size_t, N> {};
+
+template <typename T, size_t N>
+struct ExtentImpl<std::array<T, N>> : std::integral_constant<size_t, N> {};
+
+template <typename T, size_t N>
+struct ExtentImpl<base::span<T, N>> : std::integral_constant<size_t, N> {};
+
+template <typename T>
+using Extent = ExtentImpl<std::remove_cv_t<std::remove_reference_t<T>>>;
+
 template <typename T>
 struct IsSpanImpl : std::false_type {};
 
@@ -68,9 +83,10 @@ using EnableIfLegalSpanConversion =
                      IsLegalDataConversion<From, To>::value>;
 
 // SFINAE check if Array can be converted to a span<T>.
-template <typename Array, size_t N, typename T, size_t Extent>
+template <typename Array, typename T, size_t Extent>
 using EnableIfSpanCompatibleArray =
-    std::enable_if_t<(Extent == dynamic_extent || Extent == N) &&
+    std::enable_if_t<(Extent == dynamic_extent ||
+                      Extent == internal::Extent<Array>::value) &&
                      ContainerHasConvertibleData<Array, T>::value>;
 
 // SFINAE check if Container can be converted to a span<T>.
@@ -242,20 +258,19 @@ class span : public internal::ExtentStorage<Extent> {
 
   template <
       size_t N,
-      typename = internal::EnableIfSpanCompatibleArray<T (&)[N], N, T, Extent>>
+      typename = internal::EnableIfSpanCompatibleArray<T (&)[N], T, Extent>>
   constexpr span(T (&array)[N]) noexcept : span(base::data(array), N) {}
 
   template <
       size_t N,
       typename = internal::
-          EnableIfSpanCompatibleArray<std::array<value_type, N>&, N, T, Extent>>
+          EnableIfSpanCompatibleArray<std::array<value_type, N>&, T, Extent>>
   constexpr span(std::array<value_type, N>& array) noexcept
       : span(base::data(array), N) {}
 
   template <size_t N,
             typename = internal::EnableIfSpanCompatibleArray<
                 const std::array<value_type, N>&,
-                N,
                 T,
                 Extent>>
   constexpr span(const std::array<value_type, N>& array) noexcept
@@ -317,9 +332,9 @@ class span : public internal::ExtentStorage<Extent> {
   template <size_t Offset, size_t Count = dynamic_extent>
   constexpr span<T,
                  (Count != dynamic_extent
-                     ? Count
-                     : (Extent != dynamic_extent ? Extent - Offset
-                                                 : dynamic_extent))>
+                      ? Count
+                      : (Extent != dynamic_extent ? Extent - Offset
+                                                  : dynamic_extent))>
   subspan() const noexcept {
     static_assert(Extent == dynamic_extent || Offset <= Extent,
                   "Offset must not exceed Extent");
@@ -457,66 +472,33 @@ constexpr span<T> make_span(T* begin, T* end) noexcept {
   return {begin, end};
 }
 
-template <int&... ExplicitArgumentBarrier, typename T, size_t N>
-constexpr span<T, N> make_span(T (&array)[N]) noexcept {
-  return array;
-}
-
-template <int&... ExplicitArgumentBarrier, typename T, size_t N>
-constexpr span<T, N> make_span(std::array<T, N>& array) noexcept {
-  return array;
-}
-
-template <int&... ExplicitArgumentBarrier, typename T, size_t N>
-constexpr span<const T, N> make_span(const std::array<T, N>& array) noexcept {
-  return array;
-}
-
-template <int&... ExplicitArgumentBarrier,
-          typename Container,
-          typename T = std::remove_pointer_t<
-              decltype(base::data(std::declval<Container&>()))>,
-          typename = internal::EnableIfSpanCompatibleContainer<Container&, T>>
-constexpr span<T> make_span(Container& container) noexcept {
-  return container;
-}
-
-template <
-    int&... ExplicitArgumentBarrier,
-    typename Container,
-    typename T = std::remove_pointer_t<
-        decltype(base::data(std::declval<const Container&>()))>,
-    typename = internal::EnableIfSpanCompatibleContainer<const Container&, T>>
-constexpr span<T> make_span(const Container& container) noexcept {
-  return container;
-}
-
-template <size_t N,
-          int&... ExplicitArgumentBarrier,
-          typename Container,
-          typename T = std::remove_pointer_t<
-              decltype(base::data(std::declval<Container&>()))>,
-          typename = internal::EnableIfSpanCompatibleContainer<Container&, T>>
-constexpr span<T, N> make_span(Container& container) noexcept {
-  return span<T, N>(base::data(container), base::size(container));
+// make_span utility function that deduces both the span's value_type and extent
+// from the passed in argument.
+//
+// Usage: auto span = base::make_span(...);
+template <int&... ExplicitArgumentBarrier, typename Container>
+constexpr auto make_span(Container&& container) noexcept {
+  using T =
+      std::remove_pointer_t<decltype(base::data(std::declval<Container>()))>;
+  using Extent = internal::Extent<Container>;
+  return span<T, Extent::value>(std::forward<Container>(container));
 }
 
-template <
-    size_t N,
-    int&... ExplicitArgumentBarrier,
-    typename Container,
-    typename T = std::remove_pointer_t<
-        decltype(base::data(std::declval<const Container&>()))>,
-    typename = internal::EnableIfSpanCompatibleContainer<const Container&, T>>
-constexpr span<T, N> make_span(const Container& container) noexcept {
+// make_span utility function that allows callers to explicit specify the span's
+// extent, the value_type is deduced automatically. This is useful when passing
+// a dynamically sized container to a method expecting static spans, when the
+// container is known to have the correct size.
+//
+// Note: This will CHECK that N indeed matches size(container).
+//
+// Usage: auto static_span = base::make_span<N>(...);
+template <size_t N, int&... ExplicitArgumentBarrier, typename Container>
+constexpr auto make_span(Container&& container) noexcept {
+  using T =
+      std::remove_pointer_t<decltype(base::data(std::declval<Container>()))>;
   return span<T, N>(base::data(container), base::size(container));
 }
 
-template <int&... ExplicitArgumentBarrier, typename T, size_t X>
-constexpr span<T, X> make_span(const span<T, X>& span) noexcept {
-  return span;
-}
-
 }  // namespace base
 
 // Note: std::tuple_size, std::tuple_element and std::get are specialized for

base/containers/span_unittest.cc

@@ -14,6 +14,7 @@
 
 #include "base/containers/checked_iterators.h"
 #include "base/stl_util.h"
+#include "base/strings/string_piece.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -1145,6 +1146,50 @@ TEST(SpanTest, MakeStaticSpanFromContainer) {
       "the type of made_span differs from expected_span!");
 }
 
+TEST(SpanTest, MakeStaticSpanFromConstexprContainer) {
+  constexpr StringPiece str = "Hello, World";
+  constexpr auto made_span = make_span<12>(str);
+  static_assert(str.data() == made_span.data(), "Error: data() does not match");
+  static_assert(str.size() == made_span.size(), "Error: size() does not match");
+  static_assert(std::is_same<decltype(str)::value_type,
+                             decltype(made_span)::value_type>::value,
+                "Error: value_type does not match");
+  static_assert(str.size() == decltype(made_span)::extent,
+                "Error: extent does not match");
+}
+
+TEST(SpanTest, MakeSpanFromRValueContainer) {
+  std::vector<int> vector = {-1, -2, -3, -4, -5};
+  span<const int> expected_span(vector);
+  // Note: While static_cast<T&&>(foo) is effectively just a fancy spelling of
+  // std::move(foo), make_span does not actually take ownership of the passed in
+  // container. Writing it this way makes it more obvious that we simply care
+  // about the right behavour when passing rvalues.
+  auto made_span = make_span(static_cast<std::vector<int>&&>(vector));
+  EXPECT_EQ(expected_span.data(), made_span.data());
+  EXPECT_EQ(expected_span.size(), made_span.size());
+  static_assert(decltype(made_span)::extent == dynamic_extent, "");
+  static_assert(
+      std::is_same<decltype(expected_span), decltype(made_span)>::value,
+      "the type of made_span differs from expected_span!");
+}
+
+TEST(SpanTest, MakeStaticSpanFromRValueContainer) {
+  std::vector<int> vector = {-1, -2, -3, -4, -5};
+  span<const int, 5> expected_span(vector.data(), vector.size());
+  // Note: While static_cast<T&&>(foo) is effectively just a fancy spelling of
+  // std::move(foo), make_span does not actually take ownership of the passed in
+  // container. Writing it this way makes it more obvious that we simply care
+  // about the right behavour when passing rvalues.
+  auto made_span = make_span<5>(static_cast<std::vector<int>&&>(vector));
+  EXPECT_EQ(expected_span.data(), made_span.data());
+  EXPECT_EQ(expected_span.size(), made_span.size());
+  static_assert(decltype(made_span)::extent == 5, "");
+  static_assert(
+      std::is_same<decltype(expected_span), decltype(made_span)>::value,
+      "the type of made_span differs from expected_span!");
+}
+
 TEST(SpanTest, MakeSpanFromDynamicSpan) {
   static constexpr int kArray[] = {1, 2, 3, 4, 5};
   constexpr span<const int> expected_span(kArray);

base/containers/span_unittest.nc

@@ -11,6 +11,8 @@
 #include <set>
 #include <vector>
 
+#include "base/strings/string_piece.h"
+
 namespace base {
 
 class Base {
@@ -204,7 +206,7 @@ void WontCompile() {
   span<uint8_t> bytes = as_writable_bytes(make_span(v));
 }
 
-#elif defined(NCTEST_MAKE_SPAN_FROM_SET_CONVERSION_DISALLOWED)  // [r"fatal error: no matching function for call to 'make_span'"]
+#elif defined(NCTEST_MAKE_SPAN_FROM_SET_CONVERSION_DISALLOWED)  // [r"fatal error: no matching function for call to 'data'"]
 
 // A std::set() should not satisfy the requirements for conversion to a span.
 void WontCompile() {
@@ -249,13 +251,23 @@ int WontCompile() {
   return std::get<0>(s);
 }
 
-#elif defined(NCTEST_CONST_VECTOR_DEDUCES_AS_CONST_SPAN)  // [r"fatal error: no viable conversion from 'span<const int>' to 'span<int>'"]
+#elif defined(NCTEST_CONST_VECTOR_DEDUCES_AS_CONST_SPAN)  // [r"fatal error: no viable conversion from 'span<const int, \[...\]>' to 'span<int, \[...\]>'"]
 
 int WontCompile() {
   const std::vector<int> v;
   span<int> s = make_span(v);
 }
 
+#elif defined(NCTEST_STATIC_MAKE_SPAN_CHECKS_SIZE)  // [r"constexpr variable 'made_span' must be initialized by a constant expression"]
+
+// The static make_span<N>(cont) should CHECK whether N matches size(cont). This
+// should result in compilation failures when evaluated at compile time.
+int WontCompile() {
+  constexpr StringPiece str = "Foo";
+  // Intentional extent mismatch causing CHECK failure.
+  constexpr auto made_span = make_span<2>(str);
+}
+
 #endif
 
 }  // namespace base