Bug 226351 – Add copy of std::span so that we can use it pre-moving to c++20

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sam Weinig]

Add copy of std::span so that we can use it pre-moving to c++20

[Sam Weinig]

Created attachment 429917 [details]
Patch

[Sam Weinig]

Thoughts on whether we should keep this named std::span or rename it to WTF::Span?

I prefer keeping it std::span, but am open to arguments against it.

If we do keep it std::span, I need to add guards that we only use this when compiling as c++17 and use the standard one when in c++20.

[Chris Dumez]

(In reply to Sam Weinig from comment #2)
> Thoughts on whether we should keep this named std::span or rename it to
> WTF::Span?
> 
> I prefer keeping it std::span, but am open to arguments against it.
> 
> If we do keep it std::span, I need to add guards that we only use this when
> compiling as c++17 and use the standard one when in c++20.

I don't mind using std::span at all but we called std::expected WTF::Expected for some reason?

[Sam Weinig]

Created attachment 429931 [details]
Patch

[Darin Adler]

Using std::span saves us potentially a lot of work once we have C++20 across all the compilers, so I like it.

Using std::span makes us vulnerable if there are differences between what our code depends on and what shows up in any of our supported compilers and their standard libraries. We could suddenly have a lot of work to do in a hurry when some new version comes out and we want to compile with it.

Those are the arguments for and against I can think of.

[Kimmo Kinnunen]

Using WTF::Span would make it feasible to add custom code such as WTF::Vector specific constructors. Not saying this would weigh more than using the standard naming.

[Kimmo Kinnunen]

... though since std::span does not have specialisations for containers such as std::vector, it probably is not a problem as whatever the solution there is, it should be applicable to WTF containers too..

[Darin Adler]

(In reply to Kimmo Kinnunen from comment #6)
> Using WTF::Span would make it feasible to add custom code such as
> WTF::Vector specific constructors. Not saying this would weigh more than
> using the standard naming.

Can we accomplish the same thing by putting a conversion operator in WTF::Vector?

[Sam Weinig]

Looking at the implementation in libc++, it seems like as long as WTF::Vector<> conforms to the __is_span_compatible_container trait, it should just work (I believe this is sort of a pre-Ranges "hack" to allow contiguous containers to work). The constraints are currently:

// is not a specialization of span
// is not a specialization of array
// is_array_v<Container> is false,
// data(cont) and size(cont) are well formed
// remove_pointer_t<decltype(data(cont))>(*)[] is convertible to ElementType(*)[]

That said, I am having a trouble working through the MSVC error, so... :).

[Radar WebKit Bug Importer]

<rdar://problem/78833286>

[Sam Weinig]

Created attachment 430685 [details]
Patch

[Sam Weinig]

Created attachment 430686 [details]
Patch

[Sam Weinig]

Comment on attachment 430686 [details]
Patch

My general apprehension around using std:: and the issues trying to make it work have led to this version that calls it Span (or WTF::Span with a using). I can keep trying to make the std:: span work if the consensus was that was better.

[Anders Carlsson]

Out of curiosity, is there a reason this is using a Boost copy of std::span instead the one from libc++? Compatibility with GCC?

[Sam Weinig]

(In reply to Anders Carlsson from comment #14)
> Out of curiosity, is there a reason this is using a Boost copy of std::span
> instead the one from libc++? Compatibility with GCC?

Couldn't get the libc++ one to build with MSVC. (https://ews-build.webkit.org/#/builders/10/builds/94221/steps/11/logs/stdio)

[Alex Christensen]

Comment on attachment 430686 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=430686&action=review

std::expected has not been standardized but std::span has.  That's the difference between this and WTF::Expected.

Do we want to introduce a use of this as we add it, such as the one I suggested in https://bugs.webkit.org/show_bug.cgi?id=225988 ?

> Source/WTF/wtf/Span.h:3
> +//    (See accompanying file ../../LICENSE_1_0-Boost.txt or copy at

We should probably change this to just ../LICENSE_1_0-Boost.txt

[Sam Weinig]

Created attachment 430837 [details]
Patch for landing.

[Sam Weinig]

Created attachment 430843 [details]
Patch

[EWS]

Committed r278616 (238602@main): <https://commits.webkit.org/238602@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 430843 [details].

[Sam Weinig]

(In reply to Alex Christensen from comment #16)
> Comment on attachment 430686 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=430686&action=review
> 
> Do we want to introduce a use of this as we add it, such as the one I
> suggested in https://bugs.webkit.org/show_bug.cgi?id=225988 ?

I wanted to do it separate, but I did do it https://bugs.webkit.org/show_bug.cgi?id=226773.

> 
> > Source/WTF/wtf/Span.h:3
> > +//    (See accompanying file ../../LICENSE_1_0-Boost.txt or copy at
> 
> We should probably change this to just ../LICENSE_1_0-Boost.txt

Fixed. Thanks.