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. For example, this allows to catch issues like the one fixed in commit fabeca3458b38a3d8930cb0cbc866388c3f120f1.

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
ACK	l0rinc
Concept ACK	vasild, Sjors
Stale ACK	theuni

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:

#31868 (optimization: speed up block serialization by l0rinc)
#31713 (miniscript refactor: Remove unique_ptr-indirection (#30866 follow-up) by hodlinator)
#31650 (refactor: Enforce readability-avoid-const-params-in-decls by maflcko)
#31553 (cluster mempool: add TxGraph reorg functionality by sipa)
#31551 (optimization: bulk reads(32%)/writes(298%) in [undo]block [de]serialization, ~6% faster IBD by l0rinc)
#31460 (fuzz: Expand script verification flag testing to segwit v0 and tapscript by dergoegge)
#31444 (cluster mempool: add txgraph diagrams/mining/eviction by sipa)
#31363 (cluster mempool: introduce TxGraph by sipa)
#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)
#29418 (rpc: provide per message stats for global traffic via new RPC ‘getnetmsgstats’ by vasild)

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

maflcko force-pushed on Jan 14, 2025

fanquake commented at 5:12 pm on January 21, 2025: member

Rebase now that we’ve got leveldb + the sha changes in?

l0rinc commented at 5:13 pm on January 21, 2025: contributor

the UCharCast ones

maflcko force-pushed on Jan 21, 2025

maflcko marked this as ready for review on Jan 21, 2025

maflcko commented at 10:01 pm on January 21, 2025: member

Sure, rebased and worked around a fresh GCC false positive regression in 14 (fixed in 15): https://godbolt.org/z/Gcs5WznYj

DrahtBot removed the label CI failed on Jan 22, 2025

DrahtBot commented at 5:27 am on January 24, 2025: contributor

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

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.

DrahtBot added the label CI failed on Jan 24, 2025

maflcko force-pushed on Jan 24, 2025

DrahtBot removed the label CI failed on Jan 24, 2025

maflcko commented at 10:53 am on January 24, 2025: member

Rebased and updated the description with the merged UB fix it found to serve as additional motivation

fanquake requested review from theuni on Jan 24, 2025

in src/span.h:84 in fa5d857ca7 outdated

265@@ -266,14 +266,14 @@ Span<std::byte> AsWritableBytes(Span<T> s) noexcept
266 }
267 
268 template <typename V>
269-Span<const std::byte> MakeByteSpan(V&& v) noexcept
270+auto MakeByteSpan(const V& v) noexcept

theuni commented at 8:59 pm on January 24, 2025:

I played around with a way to add a template param for static extent types, but it’s not really straightforward and not worth doing here.

In the future we could potentially add specializations for types with compile-time-known sizes via concepts.

Presumably some of the streams could benefit from static extents, but that’s waaay overkill for here.

in src/span.h:112 in face06ae2f outdated

294+template <typename T, size_t N> constexpr auto UCharSpanCast(std::span<T, N> s) -> std::span<typename std::remove_pointer_t<decltype(UCharCast(s.data()))>> { return {UCharCast(s.data()), s.size()}; }
295 
296 /** Like the Span constructor, but for (const) unsigned char member types only. Only works for (un)signed char containers. */
297-template <typename V> constexpr auto MakeUCharSpan(V&& v) -> decltype(UCharSpanCast(Span{std::forward<V>(v)})) { return UCharSpanCast(Span{std::forward<V>(v)}); }
298+template <typename V> constexpr auto MakeUCharSpan(const V& v) -> decltype(UCharSpanCast(std::span{v})) { return UCharSpanCast(std::span{v}); }
299+template <typename V> constexpr auto MakeWritableUCharSpan(V&& v) -> decltype(UCharSpanCast(std::span{std::forward<V>(v)})) { return UCharSpanCast(std::span{std::forward<V>(v)}); }

theuni commented at 9:01 pm on January 24, 2025:

Unused?

maflcko commented at 8:53 pm on February 4, 2025:

Previously it was possible to make a mutable uchar span, so I wanted to preserve the feature with the new (unused) helper.

Before:

0        std::vector<std::byte> mut;
1        Span<uint8_t> mut_sp{MakeUCharSpan(mut)};

After:

0        std::vector<std::byte> mut;
1        std::span<uint8_t> mut_sp{MakeWritableUCharSpan(mut)};

But it seems fine to remove, given that it is unused and one could use UCharSpanCast on a span that points to mutable data.

However, I’d have to edit the above comment to adjust for the removal. Let me know what you prefer.

theuni commented at 9:22 pm on February 4, 2025:

No preference, whatever’s easiest here.

theuni approved

theuni commented at 9:05 pm on January 24, 2025: member

I can’t find anything to complain about. It’s well-scoped and the scripted-diff does the heavy lifting.

Need to fixup the headers to use <span> directly when possible, but that makes sense as a follow-up.

I’m looking forward to playing with static extents :)

ACK fa6f856d7146d37d51130f249ac9be9c36f7a8ad

DrahtBot requested review from vasild on Jan 24, 2025

DrahtBot requested review from l0rinc on Jan 24, 2025

theuni commented at 9:22 pm on January 24, 2025: member

Extent-friendly version of SpanPopBack if you’re interested. Mostly I was just curious to play around a bit.

0template <typename T, size_t N = std::dynamic_extent>
1T& SpanPopBack(std::span<T, N>& span)
2{
3    T& back = span.back();
4    if constexpr (N == std::dynamic_extent)
5        span = span.first(span.size() - 1);
6    else
7        span = span.template first<N - 1>();
8    return back;
9}

in src/cluster_linearize.h:78 in fa76d5aaba outdated

74@@ -75,7 +75,7 @@ class DepGraph
75      *
76      * @param depgraph   The original DepGraph that is being remapped.
77      *
78-     * @param mapping    A Span such that mapping[i] gives the position in the new DepGraph
79+     * @param mapping    A std::span such that mapping[i] gives the position in the new DepGraph

l0rinc commented at 9:19 pm on January 27, 2025:

nit: should this be An std::span now?

in src/crypto/poly1305.h:36 in fa76d5aaba outdated

32@@ -33,7 +33,7 @@ void poly1305_finish(poly1305_context *st, unsigned char mac[16]) noexcept;
33 
34 }  // namespace poly1305_donna
35 
36-/** C++ wrapper with std::byte Span interface around poly1305_donna code. */
37+/** C++ wrapper with std::byte std::span interface around poly1305_donna code. */

l0rinc commented at 9:23 pm on January 27, 2025:

nit: this new std::byte std::span looks a bit awkward now

in src/bench/load_external.cpp:48 in fa76d5aaba outdated

45@@ -46,7 +46,7 @@ static void LoadExternalBlockFile(benchmark::Bench& bench)
46     ss << static_cast<uint32_t>(benchmark::data::block413567.size());
47     // We can't use the streaming serialization (ss << benchmark::data::block413567)
48     // because that first writes a compact size.
49-    ss << Span{benchmark::data::block413567};
50+    ss << std::span{benchmark::data::block413567};

l0rinc commented at 10:05 pm on January 27, 2025:

Previously

0ss << Span{benchmark::data::block413567};

called:

0template <typename Stream, BasicByte B> void Serialize(Stream& s, Span<B> span) { s.write(AsBytes(span)); }

But after the change it calls:

0template <typename Stream, BasicByte B, std::size_t N> void Serialize(Stream& s, std::span<B, N> span) { s.write(std::as_bytes(span)); } //

The result in this case seems to be the same, so we might as well remove the extra span around the spans now:

0    ss << benchmark::data::block413567;

maflcko commented at 9:10 pm on February 4, 2025:

I think it is clearer to keep span-serialization documented here. Previously, the data was contained in a vector, which made this span constructor required (std::vector<uint8_t> block413567).

This changed in commit faecca9a85c1c1917d024f55cca34d19cc94f3b9, which forgot to update this comment. So I think a separate commit could update the comment here.

l0rinc commented at 9:59 pm on February 4, 2025:

Previously, the data was contained in a vector

A span is generated now:

0inline constexpr std::span block413567{detail_block413567_raw};

So I don’t see the reason for wrapping it in another span now and adding a historical comment which doesn’t explain the situation.

maflcko commented at 7:46 am on February 5, 2025:

I know, but this is unrelated to this pull request. std::span serialization is allowed today on current master (faaf4800aa752dde63b8987b1eb0de4e54acf717), so if someone wanted to remove this, it could be done unrelated to this pull request.

I don’t really see the risk of documenting that span serialization is intended, but I am happy to reword or remove the comment.

maflcko commented at 1:02 pm on February 5, 2025:

(Resolving for now, but I am still happy to reword or remove the comment.)

in src/test/span_tests.cpp:49 in fa76d5aaba outdated

47-// don't work. This makes it is possible to use the Span constructor in a SFINAE
48+// Make sure template std::span template deduction guides accurately enable calls to
49+// std::span constructor overloads that work, and disable calls to constructor overloads that
50+// don't work. This makes it is possible to use the std::span constructor in a SFINAE
51 // contexts like in the Spannable function above to detect whether types are or
52 // aren't compatible with Spans at compile time.

l0rinc commented at 10:20 pm on January 27, 2025:

Seems like a leftover Spans

l0rinc commented at 10:03 pm on February 4, 2025:

Done, you can resolve it (except that “As a general rule, we never use an apostrophe in writing plural forms”)

maflcko commented at 10:20 pm on February 4, 2025:

Not sure how to express it better, but I am happy to push any diff.

l0rinc commented at 10:26 pm on February 4, 2025:

Maybe something like:

0// don't work. This makes it possible to use the std::span constructor in a SFINAE
1// contexts like in the Spannable function above to detect whether types are or
2// aren't compatible with std::span at compile time.

in src/script/interpreter.h:340 in fa76d5aaba outdated

334@@ -335,13 +335,13 @@ class DeferringSignatureChecker : public BaseSignatureChecker
335 };
336 
337 /** Compute the BIP341 tapleaf hash from leaf version & script. */
338-uint256 ComputeTapleafHash(uint8_t leaf_version, Span<const unsigned char> script);
339+uint256 ComputeTapleafHash(uint8_t leaf_version, std::span<const unsigned char> script);
340 /** Compute the BIP341 tapbranch hash from two branches.
341   * Spans must be 32 bytes each. */

l0rinc commented at 10:21 pm on January 27, 2025:

should this be std::span as well?

maflcko commented at 10:26 pm on February 4, 2025:

I think it is fine. It is the short version of “The span objects must be 32 bytes each.”

l0rinc commented at 10:31 pm on February 4, 2025:

Thanks, please resolve it

theuni commented at 10:38 pm on February 4, 2025:

This looks like a good use-case for a static extent :)

(as a follow-up)

l0rinc commented at 10:34 pm on January 27, 2025: contributor

I have reviewed until fa76d5aaba34928cf403b5e142705fd03e26553f - mostly nits, only concern is a possible slight behavior change.

Could you please check if the serialization method changes that resulted from this are expected (likely applied to the array-backed spans with fixed sizes only)?

DrahtBot requested review from l0rinc on Jan 27, 2025

DrahtBot commented at 4:55 am on January 28, 2025: contributor

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

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.

DrahtBot added the label CI failed on Jan 28, 2025

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.

00000d6b97

maflcko force-pushed on Feb 4, 2025

maflcko commented at 9:29 pm on February 4, 2025: member

Rebased and added one doc-only commit to address all feedback. Should be trivial to re-review.

in src/span.h:14 in 9999fb5bd4 outdated

10@@ -11,49 +11,49 @@
11 #include <type_traits>
12 #include <utility>
13 
14-/** A Span is an object that can refer to a contiguous sequence of objects.
15+/** A std::span is an object that can refer to a contiguous sequence of objects.

l0rinc commented at 10:06 pm on February 4, 2025:

shouldn’t these go to the developer notes now? We don’t have files for other std types locally which explain their behavior.

maflcko commented at 10:25 pm on February 4, 2025:

I can see it both ways. The span.h will remain to hold std::span helpers, so it seems fine to keep it there. It is also fine to move it the dev notes, but this should be a separate commit, to make review easier. Finally, it is also fine to remove it, as the std lib has documentation and doesn’t need additional ones in this repo?

I don’t really mind either way and I think anything is fine, so I just try to make it as easy to review for reviewers.

l0rinc commented at 10:30 pm on February 4, 2025:

I find the suggestions helpful, I’m fine with moving them to doc separately. The span's should likely be adjusted here as well, maybe to instances of std::span or something similar.

maflcko commented at 12:40 pm on February 5, 2025:

Adjusted span's, so resolving this for now.

l0rinc commented at 10:21 pm on February 4, 2025: contributor

Docker seems to complain:

You have reached your pull rate limit

But locally all tests were passing for me.

I think this is an important change, I’d just a like a few minor changes (like remaining “Spans” references), and I’d prefer doing the unrelated header changes in a different PR for transparency.

DrahtBot removed the label CI failed on Feb 4, 2025

maflcko force-pushed on Feb 5, 2025

in src/test/span_tests.cpp:47 in fa04eec5ad outdated

55-// Make sure template Span template deduction guides accurately enable calls to
56-// Span constructor overloads that work, and disable calls to constructor overloads that
57-// don't work. This makes it is possible to use the Span constructor in a SFINAE
58+// Make sure template std::span template deduction guides accurately enable calls to
59+// std::span constructor overloads that work, and disable calls to constructor overloads that
60+// don't work. This makes it is possible to use the std::span constructor in a SFINAE

l0rinc commented at 9:46 am on February 5, 2025:

0// don't work. This makes it possible to use the std::span constructor in a SFINAE

maflcko commented at 9:54 am on February 5, 2025:

Thx, happy to push, but I’ll wait until you are done with your review until you reached an ack-state before pushing again. Otherwise, I’ll just spend cycles on finding the right commit to fixup the unrelated typo and then pushing it for each typo/nit separately. It is also easier for reviewers to place a single ack and a single re-ack, instead of re-acks for each typo individually.

l0rinc commented at 10:11 am on February 5, 2025:

I posted this again since I saw that the build was failing, thought you’ll need to push soon anyway.

Will the copyright changes remain part of this PR? I would prefer reviewing that separately, but if you insist, I can do it here as well.

maflcko commented at 11:01 am on February 5, 2025:

Up to you, you can skip review of the commit (and say so, and ask me to drop it), or you can review it. I’ll just wait for your ack and further comments, so that everything can be wrapped up in as few force pushes and re-reviews as possible.

The CI issue was on the CI machine, so I fixed it there and did a re-run.

l0rinc commented at 11:42 am on February 5, 2025:

Reviewed it in more detail, git diff --name-only HEAD~6..HEAD basically only contains doc/developer-notes.md in addition to the previous commit - which doesn’t contain a copyright header. The other skipped files don’t contain any dates, so it’s a correct change, nicely done.

maflcko commented at 12:58 pm on February 5, 2025:

Pushed the typo fix, so closing this for now.

in src/span.h:295 in faa0e0b66a outdated

289@@ -290,9 +290,10 @@ template <typename B>
290 concept BasicByte = requires { UCharCast(std::span<B>{}.data()); };
291 
292 // Helper function to safely convert a Span to a Span<[const] unsigned char>.
293-template <typename T> constexpr auto UCharSpanCast(Span<T> s) -> Span<typename std::remove_pointer<decltype(UCharCast(s.data()))>::type> { return {UCharCast(s.data()), s.size()}; }
294+template <typename T, size_t N> constexpr auto UCharSpanCast(std::span<T, N> s) -> std::span<typename std::remove_pointer_t<decltype(UCharCast(s.data()))>> { return {UCharCast(s.data()), s.size()}; }
295 
296 /** Like the Span constructor, but for (const) unsigned char member types only. Only works for (un)signed char containers. */

l0rinc commented at 10:45 am on February 5, 2025:

Is this still accurate, now that we’re always explicitly const-ing?

0... but for const unsigned char member types

maflcko commented at 12:37 pm on February 5, 2025:

The comment is for the second function below, see #31519 (review)

in src/random.h:403 in fa04eec5ad outdated

399@@ -400,8 +400,8 @@ class FastRandomContext : public RandomMixin<FastRandomContext>
400         return ReadLE64(buf.data());
401     }
402 
403-    /** Fill a byte Span with random bytes. This overrides the RandomMixin version. */
404-    void fillrand(Span<std::byte> output) noexcept;
405+    /** Fill a byte std::span with random bytes. This overrides the RandomMixin version. */

l0rinc commented at 11:27 am on February 5, 2025:

nit: for consistency:

0    /** Fill a byte span with random bytes. This overrides the RandomMixin version. */

maflcko commented at 12:58 pm on February 5, 2025:

thx, done

in src/span.h:108 in fa04eec5ad outdated

103@@ -289,10 +104,11 @@ inline const unsigned char* UCharCast(const std::byte* c) { return reinterpret_c
104 template <typename B>
105 concept BasicByte = requires { UCharCast(std::span<B>{}.data()); };
106 
107-// Helper function to safely convert a Span to a Span<[const] unsigned char>.
108-template <typename T> constexpr auto UCharSpanCast(Span<T> s) -> Span<typename std::remove_pointer<decltype(UCharCast(s.data()))>::type> { return {UCharCast(s.data()), s.size()}; }
109+// Helper function to safely convert a span to a span<[const] unsigned char>.
110+template <typename T, size_t N> constexpr auto UCharSpanCast(std::span<T, N> s) -> std::span<typename std::remove_pointer_t<decltype(UCharCast(s.data()))>> { return {UCharCast(s.data()), s.size()}; }

l0rinc commented at 11:28 am on February 5, 2025:

nit:

0template <typename T, size_t N> constexpr auto UCharSpanCast(std::span<T, N> s) -> std::span<std::remove_pointer_t<decltype(UCharCast(s.data()))>> { return {UCharCast(s.data()), s.size()}; }

maflcko commented at 12:58 pm on February 5, 2025:

thx, done

fanquake added this to the milestone 30.0 on Feb 5, 2025

l0rinc approved

l0rinc commented at 12:12 pm on February 5, 2025: contributor

I have recreated the changes locally and compared it against your change and reviewed the diffs one-by-one. Based on my understanding, your changes are accurate, nicely transitioning to the new type in careful commits, you have thought of every corner case.

Rebased the change against latest master, all tests are passing locally as well and I’m getting the same scripted diff results (rewritten slightly for mac).

0gsed -i "s#\<Span\>#std::span#g" $(git grep -l "Span" -- "./src" ":(exclude)src/span.h" ":(exclude)src/leveldb")
1gsed -i "s#\<AsBytes\>#std::as_bytes#g" $(git grep -l "AsBytes" -- "./src" ":(exclude)src/span.h" ":(exclude)src/leveldb")
2gsed -i "s#\<AsWritableBytes\>#std::as_writable_bytes#g" $(git grep -l "AsWritableBytes" -- "./src" ":(exclude)src/span.h" ":(exclude)src/leveldb")
3gsed -i 's#SpanPopBack(Span#SpanPopBack(std::span#g' ./src/span.h

0gsed -i --regexp-extended 's#( 20[0-2][0-9])(-20[0-2][0-9])? (The Bitcoin Core developers)#\1-present \3#g' $(git diff --name-only HEAD~6..HEAD)

I only have leftover nits (e.g. ss << benchmark::data::block413567; instead of ss << std::spam{benchmark::data::block413567}; or excluding the whole leveldb folder in the scripted diff instead of just ":(exclude)src/leveldb/db/log_test.cc"), but nothing critical.

Thanks for taking care of these refactorings!

ACK fa04eec5adc15f3e236e881fefd2376ce3ddc6a0

DrahtBot requested review from theuni on Feb 5, 2025

refactor: Return std::span from MakeUCharSpan

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

fa61a99139

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.

fa394d356b

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.

fa58342c12

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" ":(exclude)src/leveldb/db/log_test.cc" ) ; }

 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-

fa2879c207

refactor: Remove unused Span alias

Also, fixup some wording.

fa116bf5b1

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 --regexp-extended 's;( 20[0-2][0-9])(-20[0-2][0-9])? The Bitcoin Core developers;\1-present The Bitcoin Core developers;g' $( git show --pretty="" --name-only HEAD~1 )
-END VERIFY SCRIPT-

fa8eec7e3a

refactor: Avoid false-positive gcc warning

GCC 14.2.1 will complain about a dangling reference after replacing Span
wiht std::span. This is a false-positive, because std::find does not
return a reference.

Remove the `&` to silence the warning. Also use ranges::find while
touching the line.

src/i2p.cpp:312:21: error: possibly dangling reference to a temporary [-Werror=dangling-reference]
  312 |         const auto& pos = std::find(kv.begin(), kv.end(), '=');
      |                     ^~~
src/i2p.cpp:312:36: note: the temporary was destroyed at the end of the full expression ‘std::find<__gnu_cxx::__normal_iterator<const char*, span<const char> >, char>((& kv)->std::span<const char>::begin(), (& kv)->std::span<const char>::end(), '=')’
  312 |         const auto& pos = std::find(kv.begin(), kv.end(), '=');
      |                           ~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors

fa8404d62e

bench: Update span-serialize comment

Commit faecca9a85c1c1917d024f55cca34d19cc94f3b9 changed the type of
block413567 from vector to span, but forgot to update the comment. Do it
now.

fa77685ba5

maflcko force-pushed on Feb 5, 2025

l0rinc commented at 1:01 pm on February 5, 2025: contributor

ACK fa77685ba5c7185781acca04f57399cdcd19e9f7

Sjors commented at 9:17 am on February 10, 2025: member

Concept ACK. I’ll need to rebase some things, but I’m favor of getting that over with.

theuni commented at 5:15 pm on February 14, 2025: member

@fanquake do you have an opinion on this for v29? Getting it in would make future backports easier. Could also see waiting until after branch-off to be less disruptive though.

Edit: Just noticed the v30 milestone. I guess that answers my question :)

refactor: Use std::span over Span #31519

Code Coverage & Benchmarks

Reviews

Conflicts