Bug 226351 - Add copy of std::span so that we can use it pre-moving to c++20
Summary: Add copy of std::span so that we can use it pre-moving to c++20
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Weinig
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-05-27 13:22 PDT by Sam Weinig
Modified: 2021-06-08 13:08 PDT (History)
13 users (show)

See Also:


Attachments
Patch (34.62 KB, patch)
2021-05-27 13:25 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (34.71 KB, patch)
2021-05-27 14:37 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (29.81 KB, patch)
2021-06-06 09:54 PDT, Sam Weinig
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (31.57 KB, patch)
2021-06-06 09:55 PDT, Sam Weinig
achristensen: review+
Details | Formatted Diff | Diff
Patch for landing. (31.57 KB, patch)
2021-06-08 07:33 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (29.97 KB, patch)
2021-06-08 08:05 PDT, Sam Weinig
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2021-05-27 13:22:47 PDT
Add copy of std::span so that we can use it pre-moving to c++20
Comment 1 Sam Weinig 2021-05-27 13:25:46 PDT
Created attachment 429917 [details]
Patch
Comment 2 Sam Weinig 2021-05-27 14:22:46 PDT
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.
Comment 3 Chris Dumez 2021-05-27 14:36:24 PDT
(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?
Comment 4 Sam Weinig 2021-05-27 14:37:20 PDT
Created attachment 429931 [details]
Patch
Comment 5 Darin Adler 2021-05-27 15:01:58 PDT
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.
Comment 6 Kimmo Kinnunen 2021-05-27 22:51:45 PDT
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.
Comment 7 Kimmo Kinnunen 2021-05-27 22:56:34 PDT
... 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..
Comment 8 Darin Adler 2021-05-28 10:40:20 PDT
(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?
Comment 9 Sam Weinig 2021-05-29 18:34:37 PDT
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... :).
Comment 10 Radar WebKit Bug Importer 2021-06-03 13:23:21 PDT
<rdar://problem/78833286>
Comment 11 Sam Weinig 2021-06-06 09:54:21 PDT Comment hidden (obsolete)
Comment 12 Sam Weinig 2021-06-06 09:55:36 PDT Comment hidden (obsolete)
Comment 13 Sam Weinig 2021-06-07 08:32:18 PDT
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.
Comment 14 Anders Carlsson 2021-06-07 16:54:52 PDT
Out of curiosity, is there a reason this is using a Boost copy of std::span instead the one from libc++? Compatibility with GCC?
Comment 15 Sam Weinig 2021-06-07 17:39:13 PDT
(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)
Comment 16 Alex Christensen 2021-06-07 22:01:42 PDT
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
Comment 17 Sam Weinig 2021-06-08 07:33:06 PDT
Created attachment 430837 [details]
Patch for landing.
Comment 18 Sam Weinig 2021-06-08 08:05:55 PDT
Created attachment 430843 [details]
Patch
Comment 19 EWS 2021-06-08 09:54:42 PDT
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].
Comment 20 Sam Weinig 2021-06-08 10:23:15 PDT
(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.