Summary: | Add copy of std::span so that we can use it pre-moving to c++20 | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sam Weinig <sam> | ||||||||||||||
Component: | New Bugs | Assignee: | Sam Weinig <sam> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | achristensen, andersca, annulen, benjamin, cdumez, cmarcelo, darin, ews-watchlist, gyuyoung.kim, kkinnunen, ryuan.choi, sergio, webkit-bug-importer | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Attachments: |
|
Description
Sam Weinig
2021-05-27 13:22:47 PDT
Created attachment 429917 [details]
Patch
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. (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? Created attachment 429931 [details]
Patch
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. 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. ... 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.. (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? 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... :). Created attachment 430685 [details]
Patch
Created attachment 430686 [details]
Patch
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.
Out of curiosity, is there a reason this is using a Boost copy of std::span instead the one from libc++? Compatibility with GCC? (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 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 Created attachment 430837 [details]
Patch for landing.
Created attachment 430843 [details]
Patch
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]. (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. |