span: diagnose dangling views from MakeByteSpan/MakeUCharSpan #36183

Concretely this, plus a #include <ranges> at the top:

template <typename V> requires std::ranges::borrowed_range<V>
auto MakeByteSpan(const V& v) noexcept
{
    return std::as_bytes(std::span{v});
}
template <typename V> requires (!std::ranges::borrowed_range<V>)
auto MakeByteSpan(const V& v LIFETIMEBOUND) noexcept
{
    return std::as_bytes(std::span{v});
}

The first overload takes whatever std::ranges::borrowed_range accepts, std::span and std::string_view among them, and everything else takes the second. MakeUCharSpan takes the same pair, with the requires sitting alongside its decltype return.

Taken as-is, with #include <ranges> added. Only difference is that clang-format puts the requires-clause on its own line rather than inline with the decltype return for MakeUCharSpan.

Neither form keeps the helper here. MakeByteSpan(m_impl_iter->iter->key()) warns because the Slice is a temporary, and binding it to a local only changes the warning to "address of stack memory associated with local variable 'key' returned".

Would opting the Slice in be worth it? On top of those overloads it brings both getters back to the helper, and a stored MakeByteSpan of a temporary std::vector still warns. On its own it changes nothing, since the annotation as it stands is unconditional.

template<> inline constexpr bool std::ranges::enable_borrowed_range<leveldb::Slice> = true;

Opted it in, and it turns out to be load-bearing rather than just a cleanup. With the constrained overloads but no specialization, clang gives -Wreturn-stack-address on both getters, since Slice is not a borrowed_range and lands on the annotated overload. With the specialization both getters go back to MakeByteSpan and the file is clean.

I kept it in dbwrapper.cpp rather than the header: dbwrapper.h deliberately doesn't pull in leveldb headers, and this is the only TU that pairs Slice with the span helpers.

UncopyableStream inherits the DataStream constructor that copies into vch, so no view is kept here. Reverted, the file still compiles clean.

    UncopyableStream stream{MakeByteSpan(std::string_view{"abc"})};

Reverted, thanks, you're right that UncopyableStream copies into vch, so no view outlives the temporary.

unrelated: Seems odd to require BasicByte here for MakeUCharSpan, but not for MakeByteSpan.

Either it should be removed for both, or enforced for both.

Maybe it can be made explicit with something like:

#include <span>
#include <cstddef>

enum class EndianPolicy {
    Strict,
    Ignore
};

template <EndianPolicy Policy = EndianPolicy::Strict, typename V>
auto MakeByteSpan(const V& v) noexcept
{
    if constexpr (sizeof(typename decltype(std::span{v})::element_type) > 1) {
        static_assert(Policy == EndianPolicy::Ignore,
            "Cannot create byte span for multi-byte types: byte order is architecture-dependent. "
            "Use 'MakeByteSpan<EndianPolicy::Ignore>(...)' to acknowledge this.");
    }

    return std::as_bytes(std::span{v});
}

Also, UCharSpanCast is only called/used once, so maybe it can be renamed to UCharCast?

Afterwards, MakeUCharSpan(a) could even be removed and replaced by UCharCast(MakeByteSpan(a))?

also, reminds me that UCharCast will eat the annotation anyway right now. At least I don't get any warning for:

diff --git a/src/test/span_tests.cpp b/src/test/span_tests.cpp
index 42b62413aa..f0a22636d6 100644
--- a/src/test/span_tests.cpp
+++ b/src/test/span_tests.cpp
@@ -44,0 +45,6 @@ BOOST_AUTO_TEST_SUITE(span_tests)
+auto test_function()
+{
+    std::vector<std::byte> a;
+    return UCharSpanCast(std::span{a});
+}
+

So this pull is incomplete anyway