Merge #18468: Span improvements

26acc8dd9b512f220c1facdba2c5de7976d3c258 Add sanity check asserts to span when -DDEBUG (Pieter Wuille)
2676aeadfa0e43dcaaccc4720623cdfe0beed528 Simplify usage of Span in several places (Pieter Wuille)
ab303a16d114b1e94c6cf0e4c5db5389dfa197f6 Add Span constructors for arrays and vectors (Pieter Wuille)
bb3d38fc061d8482e68cd335a45c9cd8bb66a475 Make pointer-based Span construction safer (Pieter Wuille)
1f790a1147ad9a5fe06987d84b6cd71f91cbec4b Make Span size type unsigned (Pieter Wuille)

Pull request description:

  This improves our Span class by making it closer to the C++20 `std::span` one:
  * ~~Support conversion between compatible Spans (e.g. `Span<char>` to `Span<const char>`).~~ (done in #18591)
  * Make the size type `std::size_t` rather than `std::ptrdiff_t` (the C++20 one underwent the same change).
  * Support construction of Spans directly from arrays, `std::string`s, `std::array`s, `std::vector`s, `prevector`s, ... (for all but arrays, this only works for const containers to prevent surprises).

  And then make use of those improvements in various call sites.

  I realize the template magic used looks scary, but it's only needed to make overload resultion make the right choices. Note that the operations done on values are all extremely simple: no casts, explicit conversions, or warning-silencing constructions. That should hopefully make it simpler to review.

ACKs for top commit:
  laanwj:
    Code review ACK 26acc8dd9b512f220c1facdba2c5de7976d3c258
  promag:
    Code review ACK 26acc8dd9b512f220c1facdba2c5de7976d3c258.

Tree-SHA512: 5a5bd346a140edf782b5b3b3f04d9160c7b9e9def35159814a07780ab1dd352545b88d3cc491e0f80d161f829c49ebfb952fddc9180f1a56f1257aa51f38788a

6 files changed, 104 insertions, 44 deletions

diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp
index ed0175bb10..7a5421ab6f 100644
--- a/src/script/descriptor.cpp
+++ b/src/script/descriptor.cpp
@@ -139,7 +139,7 @@ std::string DescriptorChecksum(const Span<const char>& span)
     return ret;
 }
 
-std::string AddChecksum(const std::string& str) { return str + "#" + DescriptorChecksum(MakeSpan(str)); }
+std::string AddChecksum(const std::string& str) { return str + "#" + DescriptorChecksum(str); }
 
 ////////////////////////////////////////////////////////////////////////////
 // Internal representation                                                //
@@ -1087,7 +1087,7 @@ bool CheckChecksum(Span<const char>& sp, bool require_checksum, std::string& err
 
 std::unique_ptr<Descriptor> Parse(const std::string& descriptor, FlatSigningProvider& out, std::string& error, bool require_checksum)
 {
-    Span<const char> sp(descriptor.data(), descriptor.size());
+    Span<const char> sp{descriptor};
     if (!CheckChecksum(sp, require_checksum, error)) return nullptr;
     auto ret = ParseScript(0, sp, ParseScriptContext::TOP, out, error);
     if (sp.size() == 0 && ret) return std::unique_ptr<Descriptor>(std::move(ret));
@@ -1098,7 +1098,7 @@ std::string GetDescriptorChecksum(const std::string& descriptor)
 {
     std::string ret;
     std::string error;
-    Span<const char> sp(descriptor.data(), descriptor.size());
+    Span<const char> sp{descriptor};
     if (!CheckChecksum(sp, false, error, &ret)) return "";
     return ret;
 }
diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp
index 23d5b72a5c..9415bba585 100644
--- a/src/script/interpreter.cpp
+++ b/src/script/interpreter.cpp
@@ -1522,7 +1522,7 @@ static bool ExecuteWitnessScript(const Span<const valtype>& stack_span, const CS
 static bool VerifyWitnessProgram(const CScriptWitness& witness, int witversion, const std::vector<unsigned char>& program, unsigned int flags, const BaseSignatureChecker& checker, ScriptError* serror)
 {
     CScript scriptPubKey;
-    Span<const valtype> stack = MakeSpan(witness.stack);
+    Span<const valtype> stack{witness.stack};
 
     if (witversion == 0) {
         if (program.size() == WITNESS_V0_SCRIPTHASH_SIZE) {
diff --git a/src/span.h b/src/span.h
index 73b37e35f3..4931507719 100644
--- a/src/span.h
+++ b/src/span.h
@@ -10,6 +10,14 @@
 #include <algorithm>
 #include <assert.h>
 
+#ifdef DEBUG
+#define CONSTEXPR_IF_NOT_DEBUG
+#define ASSERT_IF_DEBUG(x) assert((x))
+#else
+#define CONSTEXPR_IF_NOT_DEBUG constexpr
+#define ASSERT_IF_DEBUG(x)
+#endif
+
 /** A Span is an object that can refer to a contiguous sequence of objects.
  *
  * It implements a subset of C++20's std::span.
@@ -18,12 +26,29 @@ template<typename C>
 class Span
 {
     C* m_data;
-    std::ptrdiff_t m_size;
+    std::size_t m_size;
 
 public:
     constexpr Span() noexcept : m_data(nullptr), m_size(0) {}
-    constexpr Span(C* data, std::ptrdiff_t size) noexcept : m_data(data), m_size(size) {}
-    constexpr Span(C* data, C* end) noexcept : m_data(data), m_size(end - data) {}
+
+    /** Construct a span from a begin pointer and a size.
+     *
+     * This implements a subset of the iterator-based std::span constructor in C++20,
+     * which is hard to implement without std::address_of.
+     */
+    template <typename T, typename std::enable_if<std::is_convertible<T (*)[], C (*)[]>::value, int>::type = 0>
+    constexpr Span(T* begin, std::size_t size) noexcept : m_data(begin), m_size(size) {}
+
+    /** Construct a span from a begin and end pointer.
+     *
+     * This implements a subset of the iterator-based std::span constructor in C++20,
+     * which is hard to implement without std::address_of.
+     */
+    template <typename T, typename std::enable_if<std::is_convertible<T (*)[], C (*)[]>::value, int>::type = 0>
+    CONSTEXPR_IF_NOT_DEBUG Span(T* begin, T* end) noexcept : m_data(begin), m_size(end - begin)
+    {
+        ASSERT_IF_DEBUG(end >= begin);
+    }
 
     /** Implicit conversion of spans between compatible types.
      *
@@ -42,18 +67,59 @@ public:
     /** Default assignment operator. */
     Span& operator=(const Span& other) noexcept = default;
 
+    /** Construct a Span from an array. This matches the corresponding C++20 std::span constructor. */
+    template <int N>
+    constexpr Span(C (&a)[N]) noexcept : m_data(a), m_size(N) {}
+
+    /** Construct a Span for objects with .data() and .size() (std::string, std::array, std::vector, ...).
+     *
+     * This implements a subset of the functionality provided by the C++20 std::span range-based constructor.
+     *
+     * To prevent surprises, only Spans for constant value types are supported when passing in temporaries.
+     * Note that this restriction does not exist when converting arrays or other Spans (see above).
+     */
+    template <typename V, typename std::enable_if<(std::is_const<C>::value || std::is_lvalue_reference<V>::value) && std::is_convertible<typename std::remove_pointer<decltype(std::declval<V&>().data())>::type (*)[], C (*)[]>::value && std::is_convertible<decltype(std::declval<V&>().size()), std::size_t>::value, int>::type = 0>
+    constexpr Span(V&& v) noexcept : m_data(v.data()), m_size(v.size()) {}
+
     constexpr C* data() const noexcept { return m_data; }
     constexpr C* begin() const noexcept { return m_data; }
     constexpr C* end() const noexcept { return m_data + m_size; }
-    constexpr C& front() const noexcept { return m_data[0]; }
-    constexpr C& back() const noexcept { return m_data[m_size - 1]; }
-    constexpr std::ptrdiff_t size() const noexcept { return m_size; }
-    constexpr C& operator[](std::ptrdiff_t pos) const noexcept { return m_data[pos]; }
-
-    constexpr Span<C> subspan(std::ptrdiff_t offset) const noexcept { return Span<C>(m_data + offset, m_size - offset); }
-    constexpr Span<C> subspan(std::ptrdiff_t offset, std::ptrdiff_t count) const noexcept { return Span<C>(m_data + offset, count); }
-    constexpr Span<C> first(std::ptrdiff_t count) const noexcept { return Span<C>(m_data, count); }
-    constexpr Span<C> last(std::ptrdiff_t count) const noexcept { return Span<C>(m_data + m_size - count, count); }
+    CONSTEXPR_IF_NOT_DEBUG C& front() const noexcept
+    {
+        ASSERT_IF_DEBUG(size() > 0);
+        return m_data[0];
+    }
+    CONSTEXPR_IF_NOT_DEBUG C& back() const noexcept
+    {
+        ASSERT_IF_DEBUG(size() > 0);
+        return m_data[m_size - 1];
+    }
+    constexpr std::size_t size() const noexcept { return m_size; }
+    CONSTEXPR_IF_NOT_DEBUG C& operator[](std::size_t pos) const noexcept
+    {
+        ASSERT_IF_DEBUG(size() > pos);
+        return m_data[pos];
+    }
+    CONSTEXPR_IF_NOT_DEBUG Span<C> subspan(std::size_t offset) const noexcept
+    {
+        ASSERT_IF_DEBUG(size() >= offset);
+        return Span<C>(m_data + offset, m_size - offset);
+    }
+    CONSTEXPR_IF_NOT_DEBUG Span<C> subspan(std::size_t offset, std::size_t count) const noexcept
+    {
+        ASSERT_IF_DEBUG(size() >= offset + count);
+        return Span<C>(m_data + offset, count);
+    }
+    CONSTEXPR_IF_NOT_DEBUG Span<C> first(std::size_t count) const noexcept
+    {
+        ASSERT_IF_DEBUG(size() >= count);
+        return Span<C>(m_data, count);
+    }
+    CONSTEXPR_IF_NOT_DEBUG Span<C> last(std::size_t count) const noexcept
+    {
+         ASSERT_IF_DEBUG(size() >= count);
+         return Span<C>(m_data + m_size - count, count);
+    }
 
     friend constexpr bool operator==(const Span& a, const Span& b) noexcept { return a.size() == b.size() && std::equal(a.begin(), a.end(), b.begin()); }
     friend constexpr bool operator!=(const Span& a, const Span& b) noexcept { return !(a == b); }
@@ -65,26 +131,20 @@ public:
     template <typename O> friend class Span;
 };
 
-/** Create a span to a container exposing data() and size().
- *
- * This correctly deals with constness: the returned Span's element type will be
- * whatever data() returns a pointer to. If either the passed container is const,
- * or its element type is const, the resulting span will have a const element type.
- *
- * std::span will have a constructor that implements this functionality directly.
- */
-template<typename A, int N>
-constexpr Span<A> MakeSpan(A (&a)[N]) { return Span<A>(a, N); }
-
-template<typename V>
-constexpr Span<typename std::remove_pointer<decltype(std::declval<V>().data())>::type> MakeSpan(V& v) { return Span<typename std::remove_pointer<decltype(std::declval<V>().data())>::type>(v.data(), v.size()); }
+// MakeSpan helps constructing a Span of the right type automatically.
+/** MakeSpan for arrays: */
+template <typename A, int N> Span<A> constexpr MakeSpan(A (&a)[N]) { return Span<A>(a, N); }
+/** MakeSpan for temporaries / rvalue references, only supporting const output. */
+template <typename V> constexpr auto MakeSpan(V&& v) -> typename std::enable_if<!std::is_lvalue_reference<V>::value, Span<const typename std::remove_pointer<decltype(v.data())>::type>>::type { return std::forward<V>(v); }
+/** MakeSpan for (lvalue) references, supporting mutable output. */
+template <typename V> constexpr auto MakeSpan(V& v) -> Span<typename std::remove_pointer<decltype(v.data())>::type> { return v; }
 
 /** Pop the last element off a span, and return a reference to that element. */
 template <typename T>
 T& SpanPopBack(Span<T>& span)
 {
     size_t size = span.size();
-    assert(size > 0);
+    ASSERT_IF_DEBUG(size > 0);
     T& back = span[size - 1];
     span = Span<T>(span.data(), size - 1);
     return back;
diff --git a/src/test/fuzz/span.cpp b/src/test/fuzz/span.cpp
index 4aea530ef2..f6b6e8f6f0 100644
--- a/src/test/fuzz/span.cpp
+++ b/src/test/fuzz/span.cpp
@@ -18,7 +18,7 @@ void test_one_input(const std::vector<uint8_t>& buffer)
     FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
 
     std::string str = fuzzed_data_provider.ConsumeBytesAsString(32);
-    const Span<const char> span = MakeSpan(str);
+    const Span<const char> span{str};
     (void)span.data();
     (void)span.begin();
     (void)span.end();
@@ -32,7 +32,7 @@ void test_one_input(const std::vector<uint8_t>& buffer)
     }
 
     std::string another_str = fuzzed_data_provider.ConsumeBytesAsString(32);
-    const Span<const char> another_span = MakeSpan(another_str);
+    const Span<const char> another_span{another_str};
     assert((span <= another_span) != (span > another_span));
     assert((span == another_span) != (span != another_span));
     assert((span >= another_span) != (span < another_span));
diff --git a/src/test/fuzz/spanparsing.cpp b/src/test/fuzz/spanparsing.cpp
index 8e5e7dad11..e5bf5dd608 100644
--- a/src/test/fuzz/spanparsing.cpp
+++ b/src/test/fuzz/spanparsing.cpp
@@ -12,7 +12,7 @@ void test_one_input(const std::vector<uint8_t>& buffer)
     const size_t query_size = fuzzed_data_provider.ConsumeIntegral<size_t>();
     const std::string query = fuzzed_data_provider.ConsumeBytesAsString(std::min<size_t>(query_size, 1024 * 1024));
     const std::string span_str = fuzzed_data_provider.ConsumeRemainingBytesAsString();
-    const Span<const char> const_span = MakeSpan(span_str);
+    const Span<const char> const_span{span_str};
 
     Span<const char> mut_span = const_span;
     (void)spanparsing::Const(query, mut_span);
diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp
index cf26ca3adb..257328974b 100644
--- a/src/test/util_tests.cpp
+++ b/src/test/util_tests.cpp
@@ -1829,7 +1829,7 @@ BOOST_AUTO_TEST_CASE(test_spanparsing)
 
     // Const(...): parse a constant, update span to skip it if successful
     input = "MilkToastHoney";
-    sp = MakeSpan(input);
+    sp = input;
     success = Const("", sp); // empty
     BOOST_CHECK(success);
     BOOST_CHECK_EQUAL(SpanToStr(sp), "MilkToastHoney");
@@ -1854,7 +1854,7 @@ BOOST_AUTO_TEST_CASE(test_spanparsing)
 
     // Func(...): parse a function call, update span to argument if successful
     input = "Foo(Bar(xy,z()))";
-    sp = MakeSpan(input);
+    sp = input;
 
     success = Func("FooBar", sp);
     BOOST_CHECK(!success);
@@ -1877,31 +1877,31 @@ BOOST_AUTO_TEST_CASE(test_spanparsing)
     Span<const char> result;
 
     input = "(n*(n-1))/2";
-    sp = MakeSpan(input);
+    sp = input;
     result = Expr(sp);
     BOOST_CHECK_EQUAL(SpanToStr(result), "(n*(n-1))/2");
     BOOST_CHECK_EQUAL(SpanToStr(sp), "");
 
     input = "foo,bar";
-    sp = MakeSpan(input);
+    sp = input;
     result = Expr(sp);
     BOOST_CHECK_EQUAL(SpanToStr(result), "foo");
     BOOST_CHECK_EQUAL(SpanToStr(sp), ",bar");
 
     input = "(aaaaa,bbbbb()),c";
-    sp = MakeSpan(input);
+    sp = input;
     result = Expr(sp);
     BOOST_CHECK_EQUAL(SpanToStr(result), "(aaaaa,bbbbb())");
     BOOST_CHECK_EQUAL(SpanToStr(sp), ",c");
 
     input = "xyz)foo";
-    sp = MakeSpan(input);
+    sp = input;
     result = Expr(sp);
     BOOST_CHECK_EQUAL(SpanToStr(result), "xyz");
     BOOST_CHECK_EQUAL(SpanToStr(sp), ")foo");
 
     input = "((a),(b),(c)),xxx";
-    sp = MakeSpan(input);
+    sp = input;
     result = Expr(sp);
     BOOST_CHECK_EQUAL(SpanToStr(result), "((a),(b),(c))");
     BOOST_CHECK_EQUAL(SpanToStr(sp), ",xxx");
@@ -1910,7 +1910,7 @@ BOOST_AUTO_TEST_CASE(test_spanparsing)
     std::vector<Span<const char>> results;
 
     input = "xxx";
-    results = Split(MakeSpan(input), 'x');
+    results = Split(input, 'x');
     BOOST_CHECK_EQUAL(results.size(), 4U);
     BOOST_CHECK_EQUAL(SpanToStr(results[0]), "");
     BOOST_CHECK_EQUAL(SpanToStr(results[1]), "");
@@ -1918,19 +1918,19 @@ BOOST_AUTO_TEST_CASE(test_spanparsing)
     BOOST_CHECK_EQUAL(SpanToStr(results[3]), "");
 
     input = "one#two#three";
-    results = Split(MakeSpan(input), '-');
+    results = Split(input, '-');
     BOOST_CHECK_EQUAL(results.size(), 1U);
     BOOST_CHECK_EQUAL(SpanToStr(results[0]), "one#two#three");
 
     input = "one#two#three";
-    results = Split(MakeSpan(input), '#');
+    results = Split(input, '#');
     BOOST_CHECK_EQUAL(results.size(), 3U);
     BOOST_CHECK_EQUAL(SpanToStr(results[0]), "one");
     BOOST_CHECK_EQUAL(SpanToStr(results[1]), "two");
     BOOST_CHECK_EQUAL(SpanToStr(results[2]), "three");
 
     input = "*foo*bar*";
-    results = Split(MakeSpan(input), '*');
+    results = Split(input, '*');
     BOOST_CHECK_EQUAL(results.size(), 4U);
     BOOST_CHECK_EQUAL(SpanToStr(results[0]), "");
     BOOST_CHECK_EQUAL(SpanToStr(results[1]), "foo");