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

Consider implementing P2447 #74

Closed Pesa closed 2 years ago

Pesa commented 2 years ago

Please consider implementing P2447 (std::span and the missing constructor), possibly as an opt-in optional feature since it hasn't been accepted into the C++ standard yet. Thanks!

martinmoene commented 2 years ago

Thanks for your suggestion, Davide. Will look into it.

Pesa commented 2 years ago

@martinmoene thanks!

I'm playing a bit with the new constructor (as of commit 052dad8b613aef2a14517f2899b1b438eb5decc7) and I'm getting a strange error with GCC 11.2 (with -std=c++14)

.../detail/nonstd/span-lite.hpp: In instantiation of ‘constexpr nonstd::span_lite::span<T, Extent>::span(std::initializer_list<typename std::remove_cv<_Functor>::type>) [with long unsigned int U = 18446744073709551615; typename std::enable_if<(U == nonstd::span_lite::dynamic_extent), int>::type <anonymous> = 0; T = const unsigned char; long unsigned int Extent = 18446744073709551615; typename std::remove_cv<_Functor>::type = unsigned char]’:
.../detail/nonstd/span-lite.hpp:1088:26: error: initializing ‘nonstd::span_lite::span<const unsigned char>::data_’ from ‘std::initializer_list<unsigned char>::begin’ does not extend the lifetime of the underlying array [-Werror=init-list-lifetime]
 1088 |         : data_( il.begin() )
      |                  ~~~~~~~~^~

My code looks like this:

const Foo& getFoo()
{
  static const Foo foo({'b', 'a', 'r'});
  return foo;
}

And class Foo has a constructor that takes a span<const uint8_t> and makes a copy of its content.

Curiously, make_span doesn't trigger the error (but I have to specify the type):

const Foo& getFoo()
{
  static const Foo foo(make_span<uint8_t>({'b', 'a', 'r'}));
  return foo;
}
martinmoene commented 2 years ago

Thanks, my quick look is not yet enough to solve this :)

martinmoene commented 2 years ago

Here's the example in Compiler Explorer.

Clang does not issue the warning '...’ does not extend the lifetime of the underlying array [-Winit-list-lifetime].

I'm getting the impression that GCC is trying to be a bit too helpful here.

Do you think suppressing this warning in the relevant location would be a reasonable thing to do?

Pesa commented 2 years ago

I'm getting the impression that GCC is trying to be a bit too helpful here.

Indeed, it's definitely a false positive (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102482)

Do you think suppressing this warning in the relevant location would be a reasonable thing to do?

Yeah, either suppress the warning (also suggested in comment 5 of the GCC bug) or move the assignment into the body of the constructor (suggested in comment 4)

martinmoene commented 2 years ago

@Pesa Thanks for your very specific help!

The GCC report is an interesting read.

Pesa commented 2 years ago

Thank you for quickly implementing this extension!

I'm testing commit d9a35a7ff0042049e47d535af7108045baae0be0 and it seems to work as expected. The only small problem is that the #pragma throws a warning on GCC 8 and older.

.../nonstd/span-lite.hpp:1073:33: warning: unknown option after ‘#pragma GCC diagnostic’ kind [-Wpragmas]
 # pragma GCC diagnostic ignored "-Winit-list-lifetime"
                                 ^~~~~~~~~~~~~~~~~~~~~~
.../nonstd/span-lite.hpp:1092:33: warning: unknown option after ‘#pragma GCC diagnostic’ kind [-Wpragmas]
 # pragma GCC diagnostic ignored "-Winit-list-lifetime"
                                 ^~~~~~~~~~~~~~~~~~~~~~

According to https://gcc.gnu.org/gcc-9/changes.html, the warning was introduced in GCC 9.

martinmoene commented 2 years ago

You're welcome.

I'll try and guard the #pragma :)

Pesa commented 2 years ago

Thanks. The latest commit works flawlessly. Tested with GCC 7 through 11, clang 5 through 12 (on Linux with libstdc++), and various versions of Xcode (11.3+).

Pesa commented 2 years ago

FYI the #pragma workaround for the spurious warning does not work when precompiled headers are used.

martinmoene commented 2 years ago

Thanks, it's a pity :)

"GCC #pragma does not work with precompiled header":

Pesa commented 2 years ago

How about implementing the other workaround (move the assignment to data_ into the body of the constructor) instead of the #pragma?

martinmoene commented 2 years ago

Thanks, will look at that.

martinmoene commented 2 years ago

Looks like that will require span_constexpr (C++11) to become span_constexpr14:

    template< extent_t U = Extent
        span_REQUIRES_T((
            U != dynamic_extent
        ))
    >
#if span_COMPILER_GNUC_VERSION >= 900   // prevent GCC's "-Winit-list-lifetime"
    span_constexpr14 explicit span( std::initializer_list<value_type> il ) span_noexcept
    {
        data_ = il.begin();
        size_ = il.size();
    }
#else
    span_constexpr explicit span( std::initializer_list<value_type> il ) span_noexcept
        : data_( il.begin() )
        , size_( il.size()  )
    {}
#endif

Would this be a sane approach?

Pesa commented 2 years ago

hmm true. I'm using C++14 so I didn't notice the constexpr problem. The above approach looks totally sensible to me, though I can also patch my local copy of span-lite (I would understand if you didn't want to "downgrade" the constexpr-ness to support PCHs).

martinmoene commented 2 years ago

I've no problem to implement above.

Might someone complain, next step is to make the solution configurable.

Pesa commented 2 years ago

Thanks! commit 8f7935ff4e502ee023990d356d6578b8293eda74 works nicely! I suppose this issue can be closed now.