Span improvements

[sipa]

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 Add C++17 build to Travis #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::strings, std::arrays, std::vectors, prevectors, ... (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.

[DrahtBot]

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

Conflicts

No conflicts as of last run.

[sipa]

Here is one caveat that may not be obvious: whenever you have some iterator/pointer/object that stores objects of type B (which supertype A), it should not be possible to construct a Span<A> for it, despite B* being implicitly convertible to A*. The reason is that A and B may have different sizes, so pointer arithmetic in one may not correspond to the right operations in the other.

This is implemented - here and in the C++20 std::span - by checking that arrays of B can be converted to arrays of A.

[sipa]

Rebased after #18591 (which included a commit from this PR).

[jb55]

I was trying to MakeSpan on a prevector today and I realized I needed this PR. Concept ACK. will test and ACK soon.

[jb55]

Seems to be working well on a branch I'm testing. one spooky thing I ran into was:

static std::vector<unsigned char> RandomData()
{
    uint256 r = InsecureRand256();
    return std::vector<unsigned char>(r.begin(), r.end());
}

auto d = MakeSpan(RandomData());
rb1.insert(d)
BOOST_CHECK(rb1.contains(d));

Running 12 test cases...
test/bloom_tests.cpp(519): error: in "bloom_tests/rolling_bloom": check nHits == 6 has failed [3 != 6]

*** 1 failure is detected in the test module "Bitcoin Core Test Suite"
[201] jb55@monad> ./src/test/test_bitcoin -t bloom_tests                                                                                                                                                     ~/dev/github/bitcoin/bitcoin
Running 12 test cases...
test/bloom_tests.cpp(519): error: in "bloom_tests/rolling_bloom": check nHits == 6 has failed [3 != 6]

*** 1 failure is detected in the test module "Bitcoin Core Test Suite"
[201] jb55@monad> ./src/test/test_bitcoin -t bloom_tests                                                                                                                                                     ~/dev/github/bitcoin/bitcoin
Running 12 test cases...
test/bloom_tests.cpp(519): error: in "bloom_tests/rolling_bloom": check nHits == 6 has failed [2 != 6]

*** 1 failure is detected in the test module "Bitcoin Core Test Suite"
[201] jb55@monad> ./src/test/test_bitcoin -t bloom_tests                                                                                                                                                     ~/dev/github/bitcoin/bitcoin
Running 12 test cases...
test/bloom_tests.cpp(519): error: in "bloom_tests/rolling_bloom": check nHits == 6 has failed [7 != 6]

*** 1 failure is detected in the test module "Bitcoin Core Test Suite"
[201] jb55@monad>                                                                                                                                                                                            ~/dev/github/bitcoin/bitcoin
[201] jb55@monad> ./src/test/test_bitcoin -t bloom_tests                                                                                                                                                     ~/dev/github/bitcoin/bitcoin
Running 12 test cases...
test/bloom_tests.cpp(519): error: in "bloom_tests/rolling_bloom": check nHits == 6 has failed [4 != 6]

*** 1 failure is detected in the test module "Bitcoin Core Test Suite"

but this works fine:

std::vector<unsigned char> d = RandomData();
rb1.insert(MakeSpan(d));
BOOST_CHECK(rb1.contains(MakeSpan(d)));

some weird rvalue voodoo going on?

[aminroosta]

I may be wrong, but i think in your first example, the return value of RandomData() is destructed immediately.

auto d = MakeSpan(RandomData());
// d contains dangling pointers by this line.
rb1.insert(d)
BOOST_CHECK(rb1.contains(d));

MakeSpan takes a universal reference (V&&) in this PR.

because the return value of RandomData() is an Rvalue, it goes out of scope on the first line and therefore is destructed immediately.

[jb55]

@aminroosta I understand some of those words. thanks!

[sipa]

Yes, exactly what @aminroosta said.

[practicalswift]

@theuni

It would be really easy to miss in review, though :(

Agreed! It is very easy to get things wrong when reasoning about lifetime issues. This is really subtle topic filled with gotchas.

The introduction of std::span and std::string_view will bring a new golden age of use-after-frees/returns to C++ :)

Personally I'm not convinced that it is a good idea to use std::span or std::string_view as anything other than parameter types in the general case (there are exceptions obviously, but the security/stability vs performance trade-off should be considered in such cases).

As a rule of thumb: if the median reviewer has to think more than five seconds to answer "no" to the question "might there be a life-time issue hiding in this use of std::{span,string_view}?" then it is likely not a good idea to use them :)

Somewhat related: "std::string_view encourages use-after-free; the Core Guidelines Checker doesn't complain"

[jonatack]

Thanks @sipa, this PR led me to learn more about Span and C++20 std::span via these resources, among the online ones:

1. C++ std:span: https://en.cppreference.com/w/cpp/container/span

2. Yesterday's Review Club notes/questions/meeting log: https://bitcoincore.reviews/18468

3. The C++ Core Guidelines site seems to be a source of interesting recommendations on when, why, and how to use std::span: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines. It suggests using std::span in a fair number of places/reasons, among others:

• expressing intent
• improved code readability and tool support
• avoiding range errors and array decay to a pointer (losing its size, opening the opportunity for range errors; using span preserves size information)
• going further, avoiding passing arrays as a single pointer, as (pointer, size)-style interfaces are error-prone and a plain pointer (to array) must rely on some convention to allow the callee to determine the size
• potentially doing more at compile time vs run time
• potentially checking/catching run time errors earlier in the code
• abstraction for encapsulating messy constructs, rather than spreading them throughout the code
• reducing the number of function arguments, e.g. from pointer + size to a bounds-checked span representing a range
• span<T> or a span_p<T> ideal for designating half-open sequences, as informal/non-explicit ranges are a source of errors
• helpful for using abstract classes as interfaces when complete separation of interface and implementation is needed

This PR debug-builds cleanly with gcc and clang, and the fuzzers build and run.

ACK 26acc8d (AFAICT), modulo the various follow-ups suggested above.

[jonatack]

MakeSpan takes a universal reference (V&&) in this PR.

@aminroosta Thank you -- I found your link (and the downloadable presentation slides) really informative.