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:

#31650 (refactor: Enforce readability-avoid-const-params-in-decls by maflcko)
#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: batch XOR operations 12% faster IBD 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)

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

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

Concept ACK

maflcko force-pushed on Dec 19, 2024

achow101 referenced this in commit e6f14241f6 on Dec 30, 2024

maflcko force-pushed on Jan 2, 2025

maflcko force-pushed on Jan 3, 2025

refactor: Drop unused UCharCast

This is no longer needed after commit
6aa0e70ccbd5491ec9d7c81892820f3341ccd631

fad4032b21

refactor: Avoid UB in SHA3_256::Write

It is UB to apply a distance to a pointer or iterator further than the
end itself, even if the distance is (partially) revoked later on.

Fix the issue by advancing the data pointer at most to the end.

fabeca3458

leveldb_bla cf2c7064bb

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.

1bb6d97de3

refactor: Return std::span from MakeUCharSpan

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

f18c7a30f0

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.

0af256f901

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.

078ce53f8d

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-

b8f6d09ab5

scripted-diff: Bump copyright headers after std::span changes

Historically, the headers have been bumped some time after a file has
been touched. Do it now to avoid having to touch them again in the
future for that reason.

-BEGIN VERIFY SCRIPT-
 sed -i 's/-20[0-2][0-9] The Bitcoin Core developers/-present The Bitcoin Core developers/g' $( git show --pretty="" --name-only HEAD )
-END VERIFY SCRIPT-

d2808888a4

refactor: Remove unused Span alias a275504eea

maflcko force-pushed on Jan 14, 2025

refactor: Use std::span over Span #31519

Code Coverage & Benchmarks

Reviews

Conflicts