refactor: Use std::span over Span

maflcko commented at 9:56 pm on December 17, 2024: member

Span has some issues:

It does not support fixed-size spans, which are available through std::span.
It is confusing to have it available and in use at the same time with std::span.
It does not obey the standard library iterator build hardening flags. See #31272 for a discussion.

All of the issues are harmless, because both types are type-safe and can even implicitly convert into each other in most contexts.

However, exclusively using std::span seems less confusing, so do it here with a scripted-diff.

DrahtBot commented at 9:56 pm on December 17, 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/31519.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	l0rinc, vasild

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#31460 (fuzz: Expand script verification flag testing to segwit v0 and tapscript by dergoegge)
#31250 (wallet: Disable creating and loading legacy wallets by achow101)
#31244 (descriptors: MuSig2 by achow101)
#31243 (descriptor: Move filling of keys from DescriptorImpl::MakeScripts to PubkeyProvider::GetPubKey by achow101)
#31144 (optimization: speed up XOR by 4% (9% when disabled) by applying it in larger batches by l0rinc)
#30988 (Split CConnman by vasild)
#30987 (Don’t zero-after-free DataStream: Faster IBD on some configurations by davidgumberg)
#29675 (wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys by achow101)
#29307 (util: explicitly close all AutoFiles that have been written by vasild)
#29270 (Implement OP_CHECKSIGFROMSTACK(VERIFY) by reardencode)
#29269 (Add OP_INTERNALKEY for Tapscript by reardencode)
#28710 (Remove the legacy wallet and BDB dependency by achow101)
#25722 (refactor: Use util::Result class for wallet loading by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot renamed this:
~~refactor: Use std::span over Span~~
refactor: Use std::span over Span
on Dec 17, 2024

DrahtBot added the label Refactoring on Dec 17, 2024

maflcko commented at 9:56 pm on December 17, 2024: member

The CI will fail due to the subtree update. If you are interested in this change, please review https://github.com/bitcoin-core/leveldb-subtree/pull/45 first.

DrahtBot added the label CI failed on Dec 17, 2024

DrahtBot commented at 10:02 pm on December 17, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/34562140227

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

maflcko force-pushed on Dec 18, 2024

in src/span.h:253 in d60e267a3e outdated

249@@ -250,7 +250,7 @@ T& SpanPopBack(Span<T>& span)
250     size_t size = span.size();
251     ASSERT_IF_DEBUG(size > 0);
252     T& back = span[size - 1];
253-    span = Span<T>(span.data(), size - 1);
254+    span = span.first(size-1);

l0rinc commented at 12:37 pm on December 18, 2024:

It looks like this method can be slightly modernized now:

0    ASSERT_IF_DEBUG(!span.empty());
1    T& back = span.back();
2    span = span.first(span.size() - 1);

maflcko commented at 3:02 pm on December 18, 2024:

thx, done

in src/streams.h:82 in 37aa959fbd outdated

78@@ -79,7 +79,7 @@ class VectorWriter
79             memcpy(vchData.data() + nPos, src.data(), nOverwrite);
80         }
81         if (nOverwrite < src.size()) {
82-            vchData.insert(vchData.end(), UCharCast(src.data()) + nOverwrite, UCharCast(src.end()));
83+            vchData.insert(vchData.end(), UCharCast(src.data()) + nOverwrite, UCharCast(src.data()+src.size()));

l0rinc commented at 12:41 pm on December 18, 2024:

nit: formatting

0            vchData.insert(vchData.end(), UCharCast(src.data()) + nOverwrite, UCharCast(src.data() + src.size()));

in src/span.h:86 in 257fa5c8d0 outdated

266@@ -267,14 +267,14 @@ Span<std::byte> AsWritableBytes(Span<T> s) noexcept
267 }
268 
269 template <typename V>
270-Span<const std::byte> MakeByteSpan(V&& v) noexcept
271+auto MakeByteSpan(const V& v) noexcept
272 {
273-    return AsBytes(Span{std::forward<V>(v)});
274+    return std::as_bytes(std::span{v});

l0rinc commented at 12:52 pm on December 18, 2024:

Q: we don’t need perfect forwarding anymore because we can const the read-only version?

maflcko commented at 2:36 pm on December 18, 2024:

There is nothing to be forwarded, so it seems clearer not doing it. You can think of a span as an object that stores a pointer and a size. Obtaining a pointer and a size can be done from a const reference as well, especially if only a pointer to const memory is needed.

in src/span.h:121 in 02ae908080 outdated

290@@ -291,9 +291,10 @@ template <typename B>
291 concept BasicByte = requires { UCharCast(std::span<B>{}.data()); };
292 
293 // Helper function to safely convert a Span to a Span<[const] unsigned char>.

l0rinc commented at 12:57 pm on December 18, 2024:

comments here need to be updated, too

maflcko commented at 3:45 pm on December 18, 2024:

thx, done

in src/test/span_tests.cpp:58 in b66694f7b8 outdated

63@@ -64,7 +64,7 @@ BOOST_AUTO_TEST_CASE(span_constructor_sfinae)
64     BOOST_CHECK(!Spannable(std::set<int>{}));
65     BOOST_CHECK(!Spannable(std::vector<bool>{}));
66     BOOST_CHECK(Spannable(std::array<int, 3>{}));
67-    BOOST_CHECK(Spannable(Span<int>{}));
68+    BOOST_CHECK(Spannable(std::span<int>{}));
69     BOOST_CHECK(Spannable("char array"));
70     BOOST_CHECK(Spannable(SpannableYes{}));

l0rinc commented at 1:25 pm on December 18, 2024:

It seems to me that SpannableYes needs a begin/end now, not a data/size (SpannableNo should also be updated accordingly)

0struct SpannableYes
1{
2    const char* begin();
3    const char* end();
4};

maflcko commented at 12:38 pm on December 19, 2024:

This test and the commit 6d43aad742c7ea28303cf2799528188938e7ce32 that introduced it actually have many issues:

std::vector<bool>::data never returned void*, only void, see https://godbolt.org/z/ePqGxjovv
The reason why std::vector<bool> is not spannable is because the iterator is not contiguous, not due to the type of the data pointer
std::span is more correct in that it does all the checks, so my recommendation would be to just remove the test and all related code, as explained in #28721 (review)

l0rinc commented at 1:25 pm on December 18, 2024: contributor

Concept ACK

Thank you for doing these big-bang cleanups!

I understand it’s still a draft, but consider the following code parts that still need to be touched:

maflcko force-pushed on Dec 18, 2024

l0rinc commented at 3:28 pm on December 18, 2024: contributor

Can FuzzedDataProvider and CHMAC_SHA256 take a const std::span<const uint8_t> parameter now instead of separate data and size - or is that out of scope?

maflcko force-pushed on Dec 18, 2024

maflcko commented at 3:48 pm on December 18, 2024: member

Can FuzzedDataProvider and CHMAC_SHA256 take a const std::span<const uint8_t> parameter now instead of separate data and size - or is that out of scope?

I want to keep the scope minimal here. It is already more than 500 lines modified, albeit most of them trivially. In theory I could even limit the scope a bit more and defer the scripted-diff (and later commits) for later.

maflcko force-pushed on Dec 19, 2024

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

maflcko force-pushed on Dec 19, 2024

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

leveldb_bla f594801703

refactor: Return std::span from MakeByteSpan

In theory this commit should only touch the span.h header, because
std::span can implicilty convert into Span in most places, if needed.

However, at least when using the clang compiler, there are some
false-positive lifetimebound warnings and some implicit conversions can
not be resolved.

Thus, this refactoring commit also changed the affected places to
replace Span with std::span.

ff8d519d6e

maflcko force-pushed on Dec 19, 2024

vasild commented at 6:36 pm on December 19, 2024: contributor

Concept ACK

refactor: Return std::span from MakeUCharSpan

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

dedce801b3

test: Fix broken span_tests

* The comment is wrong claiming that void* was returned when void was
  returned in reality.
* The namespace is missing a name, leading to compile errors that are
  suppressed with non-standard pragmas, and leading to compile errors in
  future commits. Instead of using more non-standard suppressions, just
  add the missing name.
* The SpanableYes/No types are missing begin/end iterators, which will
  be needed when using std::span.

8c30fe3dfd

refactor: Make Span an alias of std::span

This uses a macro, which can be a bit more brittle than an alias
template. However, class template argument deduction for alias templates
is only implemented in clang-19.

2146e33e7c

scripted-diff: Use std::span over Span

-BEGIN VERIFY SCRIPT-

 ren() { sed -i "s!\<$1\>!$2!g" $( git grep -l "$1" -- "./src" ":(exclude)src/span.h" ) ; }

 ren Span            std::span
 ren AsBytes         std::as_bytes
 ren AsWritableBytes std::as_writable_bytes

 sed -i 's!SpanPopBack(Span!SpanPopBack(std::span!g' ./src/span.h

-END VERIFY SCRIPT-

c195507e09

refactor: Remove unused Span alias a0989fca2e

maflcko force-pushed on Dec 19, 2024

refactor: Use std::span over Span #31519

Code Coverage & Benchmarks

Reviews

Conflicts