martinmoene / span-lite

span lite - A C++20-like span for C++98, C++11 and later in a single-file header-only library
Boost Software License 1.0
495 stars 40 forks source link

Constexpr construction from span of same ElementType #72

Closed CrustyAuklet closed 2 years ago

CrustyAuklet commented 2 years ago

On MSVC when passing a span with static size to a function with a span of dynamic extent as a parameter, the constructor is not constexpr. The same code seems to compile fine on GCC. In my local project, MSVC errors seem to point to the reinterpret_cast on line 1092.

If I create another constructor for spans with the same ElementType, and just directly copy the data pointer, then the error goes away.

Here is a extremely simplified example compiler explorer

#include <cstdint>
#include <https://raw.githubusercontent.com/CrustyAuklet/span-lite/master/include/nonstd/span.hpp>
using nonstd::span;
using size_type = std::size_t;
using byte_type = std::uint8_t;

/// Helper function to insert padding fields into the buffer for `pack_into()`
constexpr auto insert_padding(span<byte_type> buffer, const size_type start_bit) {
    buffer[0] = 0x01 >> start_bit;
}

template<size_type N>
constexpr void pack_impl(span<byte_type, N> output, const size_type start_bit) {
    insert_padding(output, start_bit);
}

constexpr auto pack() {
    std::array<byte_type, 8> output{};
    pack_impl(span(output), 0);
    return output;
}

constexpr auto data = pack();

Here is my fork again with the naive fix. And the same compiler explorer example using the modified header.

I haven't decided if I want to use this technique yet, but the original intent is so that I can do static asserts on the span/buffer size at compile time based on the format string, then pass it on to the more generic function. I was originally using std::array in one of my functions but it was pointed out in https://github.com/CrustyAuklet/bitpacker/issues/9 that I should be taking a span.

** edit to add: It does work on both MSVC and GCC using c++20 std::span compiler explorer

martinmoene commented 2 years ago

Thanks for this interesting case.

Trying to minimize the example on compiler explorer, I stumble into MSVC accepting it and GCC not, when using C++20 with std::span.

Do you have an idea how to proceed?

CrustyAuklet commented 2 years ago

for your example, this looks like it's using aggregate initialization, correct? With your example I get the same issue on C++17, C++20, when using std::span, nonstd::span and gsl::span, tcb::span. This kind of makes sense I guess since in your example I think the type is span<int,7> all the way through.

error: 'nonstd::span_lite::span<int, 7>{((int*)(& arr.std::array<int, 7>::_M_elems)), 7}' is not a constant expression

Is this a..... compiler bug? I've never seen one of those before, at least outside my esoteric embedded compilers.


My issue specifically occurs when passing a span<int,7 to a span<int>, so it calls the copy constructor for spans with different types and extents. My origional "fix" was to make an additional constructor that had only the extent as a template parameter. Taking a step back, is it reasonable to have conversions between spans of different types? is std::is_convertable a strong enough requirement? If it is, maybe the reinterpret_cast isn't needed at all?

I did some quick tests and it seems like std:is_convertable is almost too strong. I would assume that all these types that are OK for raw memory access would be convertible but it doesn't seem so.

static_assert(std::is_convertible<uint8_t(*)[], uint8_t(*)[]>::value); // passes
static_assert(std::is_convertible<uint8_t(*)[], unsigned char(*)[]>::value); // passes
static_assert(std::is_convertible<uint8_t(*)[], char(*)[]>::value); // fails
static_assert(std::is_convertible<uint8_t(*)[], std::byte(*)[]>::value); // fails

Is there a downside to removing the reinterpret_cast in your opinion?

martinmoene commented 2 years ago

Ah, thank you for investigating and explaining!

The requirement is_­convertible_­v<OtherElementType(*)[], element_­type(*)[]> stems from the specificationin the C++ standard, which I try to follow.

You're right to question the presence of reinterpret_cast and I think it can be removed. Can't remember why I put it there.

Looking at the specification in the standard, I notice || OtherExtent == dynamic_extent is missing from the requires clause.

I'll update the constructor to:

    template< class OtherElementType, extent_type OtherExtent
        span_REQUIRES_T((
            (Extent == dynamic_extent || OtherExtent == dynamic_extent || Extent == OtherExtent)
            && std::is_convertible<OtherElementType(*)[], element_type(*)[]>::value
        ))
    >
    span_constexpr_exp span( span<OtherElementType, OtherExtent> const & other ) span_noexcept
        : data_( other.data() )
        , size_( other.size() )
    {
        span_EXPECTS( OtherExtent == dynamic_extent || other.size() == to_size(OtherExtent) );
    }