refactor: std::span compat fixes #31540

pull maflcko wants to merge 7 commits into bitcoin:master from maflcko:2412-span-prep changing 11 files +17 −20

maflcko commented at 12:57 pm on December 19, 2024: member

The std::span type is already used in some parts of the codebase, and in most contexts can implicitly convert to and from Span. However, the two types are not identical in behavior and trying to use one over the other can result in compile failures in some contexts.

Fix all those issues by allowing either Span or std::span in any part of the codebase.

All of the changes are also required for the scripted-diff to replace Span with std::span in https://github.com/bitcoin/bitcoin/pull/31519
refactor: Avoid needless, unsafe c-style cast fac3a782ea

refactor: Replace fwd-decl with proper include

This is fine, because the span.h include is lightweight and a proper
include will be needed anyway when switching to std::span.

facc4f120b

refactor: Simplify SpanPopBack

Use the equivalent back() and first() member functions.

faae6fa5f6

DrahtBot commented at 12:57 pm on December 19, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31540.

Reviews

See the guideline for information on the review process.

Type Reviewers

ACK theuni, adamandrews1

Stale ACK sipa

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
DrahtBot added the label Refactoring on Dec 19, 2024

Type	Reviewers
ACK	theuni, adamandrews1
Stale ACK	sipa

in src/script/miniscript.h:1801 in fa1be42772 outdated

1797@@ -1798,7 +1798,7 @@ inline NodeRef<Key> Parse(Span<const char> in, const Ctx& ctx)
1798         // Get threshold
1799         int next_comma = FindNextChar(in, ',');
1800         if (next_comma < 1) return false;
1801-        const auto k_to_integral{ToIntegral<int64_t>(std::string_view(in.begin(), next_comma))};
1802+        const auto k_to_integral{ToIntegral<int64_t>(std::string_view(&in.front(), next_comma))};

sipa commented at 1:20 pm on December 19, 2024:

Would in.data() work here instead of &in.front()? That expresses the intent better I think, because we don’t actually just want the first element.

maflcko commented at 2:54 pm on December 19, 2024:

thx, done

sipa commented at 1:22 pm on December 19, 2024: member

utACK fa163e04ca62b832650c9c942ebe240ad46351c8

refactor: Avoid passing span iterators when data pointers are expected

For Span, iterators are just raw data pointers. However, for std::span
they are not.

This change makes it explicit where data pointers are expected.

Otherwise, there could be a compile error later on:

  No known conversion from 'iterator' (aka '__normal_iterator<const std::byte *, std::span<const std::byte, 18446744073709551615>>') to 'std::byte *'.

fa86223475

refactor: test: Return std::span from StringBytes

This is possible and safe, because std::span can implicitly convert into
Span, if needed.

Changing this function is required, because std::span requires the
extent template parameter to be specified as well.

Instead of explicilty specifying them, just let the compiler derive the
template parameters correctly.

Otherwise, there would be a compile error later on:

 src/wallet/test/db_tests.cpp:39:37: error: no matching function for call to ‘as_bytes<const char>(<brace-enclosed initializer list>)’
 ...
 /usr/include/c++/11/span:420:5: note: candidate: ...
       |     as_bytes(span<_Type, _Extent> __sp) noexcept
       |     ^~~~~~~~
 /usr/include/c++/11/span:420:5: note:   template argument deduction/substitution failed:
 src/wallet/test/db_tests.cpp:39:37: note:   couldn’t deduce template parameter ‘_Extent’
       |     return std::as_bytes<const char>({str.data(), str.size()});
       |            ~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~

faa5391f77

Allow std::span in stream serialization

Future code can now use std::span in stream serialization, similar to
the existing serialization helpers for std::array or Span.

faaf4800aa

refactor: Specify const in std::span constructor, where needed

The std::span constructor requires std::ranges::borrowed_range, which
tries to protect against dangling references.

One way to disable the check is to specify the std::span's element type
as const in the constructor call.

Otherwise, a compile error will look like:

 include/c++/span: note: candidate constructor not viable: no known conversion from 'std::vector<unsigned char>' to 'const span<unsigned char>' for 1st argument
      |       span(const span&) noexcept = default;
      |       ^    ~~~~~~~~~~~
 ...
 include/c++/span: note: candidate template ignored: constraints not satisfied [with _Range = std::vector<unsigned char>]
      |         span(_Range&& __range)
      |         ^
 include/c++/span: note: because 'std::vector<unsigned char>' does not satisfy 'borrowed_range'
      |           && (ranges::borrowed_range<_Range> || is_const_v<element_type>)
      |               ^
 include/c++/bits/ranges_base.h: note: because 'std::vector<unsigned char>' does not satisfy '__maybe_borrowed_range'
      |       = range<_Tp> && __detail::__maybe_borrowed_range<_Tp>;
      |                       ^
 include/c++/bits/ranges_base.h: note: because 'is_lvalue_reference_v<std::vector<unsigned char> >' evaluated to false
      |         = is_lvalue_reference_v<_Tp>
      |           ^
 include/c++/bits/ranges_base.h: note: and 'enable_borrowed_range<remove_cvref_t<vector<unsigned char, allocator<unsigned char> > > >' evaluated to false
      |           || enable_borrowed_range<remove_cvref_t<_Tp>>;
      |              ^
 include/c++/span: note: and 'is_const_v<element_type>' evaluated to false
      |           && (ranges::borrowed_range<_Range> || is_const_v<element_type>)
      |                                                 ^

fa494a1d53

maflcko force-pushed on Dec 19, 2024

in src/base58.cpp:142 in fac3a782ea outdated

138@@ -139,7 +139,7 @@ std::string EncodeBase58Check(Span<const unsigned char> input)
139     // add 4-byte hash check to the end
140     std::vector<unsigned char> vch(input.begin(), input.end());
141     uint256 hash = Hash(vch);
142-    vch.insert(vch.end(), (unsigned char*)&hash, (unsigned char*)&hash + 4);

theuni commented at 3:37 pm on December 19, 2024:

😬

in src/span.h:251 in faae6fa5f6 outdated
247@@ -248,9 +248,8 @@ template <typename T> 248 T& SpanPopBack(Span<T>& span) 249 { 250 size_t size = span.size(); 251- ASSERT_IF_DEBUG(size > 0);
theuni commented at 3:42 pm on December 19, 2024:

Why drop the assert? Though I have no idea who/what tests with -DDEBUG anyway.
maflcko commented at 4:10 pm on December 19, 2024:

The assert is not dropped. In fact, there are now two asserts:

https://github.com/bitcoin/bitcoin/blob/fa494a1d53f3f030fafe7b533d72b2200428a0fd/src/span.h#L184

https://github.com/bitcoin/bitcoin/blob/fa494a1d53f3f030fafe7b533d72b2200428a0fd/src/span.h#L207

The first one is the one you left a comment on. And the second one is left for the compiler to drop, if it wants to.
theuni approved
theuni commented at 3:58 pm on December 19, 2024: member

utACK fa494a1d53f3f030fafe7b533d72b2200428a0fd.

I haven’t verified that this assortment of changes is all that’s needed for the conversion, but they all look good to me.
DrahtBot requested review from sipa on Dec 19, 2024
adamandrews1 approved
adamandrews1 commented at 4:03 pm on December 19, 2024: none

utACK https://github.com/bitcoin/bitcoin/commit/fa494a1d53f3f030fafe7b533d72b2200428a0fd

Contributors
maflcko DrahtBot sipa theuni adamandrews1

Review Requested
sipa

Labels
Refactoring

refactor: std::span compat fixes #31540

Code Coverage & Benchmarks

Reviews