Bug 197131 - Start using C++17
Summary: Start using C++17
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: Alex Christensen
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-04-19 18:16 PDT by Alex Christensen
Modified: 2019-04-26 09:47 PDT (History)
5 users (show)

See Also:


Attachments
Patch (30.96 KB, patch)
2019-04-19 18:18 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (31.91 KB, patch)
2019-04-19 20:17 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews125 for ios-simulator-wk2 (3.86 MB, application/zip)
2019-04-20 01:44 PDT, Build Bot
no flags Details
Patch (23.63 KB, patch)
2019-04-23 14:33 PDT, Alex Christensen
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2019-04-19 18:16:35 PDT
Start using C++17
Comment 1 Alex Christensen 2019-04-19 18:18:32 PDT
Created attachment 367859 [details]
Patch
Comment 2 Alex Christensen 2019-04-19 20:17:45 PDT
Created attachment 367867 [details]
Patch
Comment 3 Build Bot 2019-04-20 01:44:16 PDT
Comment on attachment 367867 [details]
Patch

Attachment 367867 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11939188

New failing tests:
http/tests/paymentrequest/payment-response-payerName-attribute.https.html
http/tests/paymentrequest/payment-response-payerPhone-attribute.https.html
http/tests/paymentrequest/updateWith-method-pmi-handling.https.html
http/tests/paymentrequest/payment-request-change-shipping-address.https.html
http/tests/paymentrequest/payment-response-complete-method.https.html
http/tests/paymentrequest/payment-address-attributes-and-toJSON-method.https.html
http/tests/paymentrequest/payment-request-change-shipping-option.https.html
http/tests/paymentrequest/payment-response-payerEmail-attribute.https.html
http/tests/paymentrequest/payment-response-methodName-attribute.https.html
http/tests/paymentrequest/payment-response-retry-method.https.html
http/tests/paymentrequest/payment-request-merchant-validation.https.html
http/tests/paymentrequest/payment-request-show-method.https.html
Comment 4 Build Bot 2019-04-20 01:44:18 PDT
Created attachment 367883 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6
Comment 5 Darin Adler 2019-04-20 07:57:14 PDT
Comment on attachment 367867 [details]
Patch

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

The "payment request" crashes in the iOS Simulator EWS results look like they might be real results of this change.

> Source/JavaScriptCore/inspector/InspectorBackendDispatcher.h:-114
> -    // This is necessary for some versions of Safari. Remove it when those versions of Safari are no longer supported.
> -    void reportProtocolError(WTF::DeprecatedOptional<long> relatedRequestId, CommonErrorCode, const String& errorMessage);

Does this really have a dependency on C++17? Can we land this change first/separately?

> Source/WTF/wtf/DeprecatedOptional.h:-48
> -template<typename T> using DeprecatedOptional = std::optional<T>;

Same question. Can we remove this header separately, first?
Comment 6 Alex Christensen 2019-04-22 08:36:07 PDT
(In reply to Darin Adler from comment #5)
> Does this really have a dependency on C++17? Can we land this change
> first/separately?
I think this is caused by there being some C++17 headers that newly include <optional>, which conflicts with this implementation of std::optional.  I don't think it's still needed, so I just removed it.  I'll actually remove it in a separate patch.
Comment 7 Michael Catanzaro 2019-04-23 08:28:28 PDT
I guess removing WTF::Optional will come next, right?

Also, it looks like you are building XCode with GNU language extensions enabled (-std=gnu++17) while we build CMake with GNU extensions disabled (-std=c++17). It makes sense to use the same language standard for all ports, so I'd suggest switching XCode to use -std=c++17 unless you are intentionally using GNU language extensions somewhere in Apple-specific code, in which case we could switch CMake ports to -std=gnu++17. (Clearly, none of the cross-platform code needs this, since CMake ports build without. Just slightly funny for the Apple ports to use GNU stuff that the Linux ports have disabled.)
Comment 8 Alex Christensen 2019-04-23 08:30:35 PDT
That is indeed ironic.  I'm not aware of any intentional use of gnu extensions.  I agree we should all try to use the same language type.

Unfortunately, there are some who want to continue using WTF::Optional because it resets to WTF::nullopt when moved from, while std::optional typically does not.
Comment 9 Alex Christensen 2019-04-23 14:31:01 PDT
Mac doesn't build with non-gnu c++17 right now.  We can make that change later.
Comment 10 Alex Christensen 2019-04-23 14:33:11 PDT
Created attachment 368065 [details]
Patch
Comment 11 Darin Adler 2019-04-25 09:20:33 PDT
Comment on attachment 368065 [details]
Patch

I’m not sure I understand the status of ports that are compiling with older versions of gcc, otherwise looks fine.
Comment 12 Alex Christensen 2019-04-25 10:40:45 PDT
(In reply to Darin Adler from comment #11)
> Comment on attachment 368065 [details]
> Patch
> 
> I’m not sure I understand the status of ports that are compiling with older
> versions of gcc, otherwise looks fine.

This will allow them to use C++17.  If they want to use -std=c++1z instead of -std=c++17 on linux and use GCC 6 with partial support of C++17, they are free to do so.  As it is on Mac when I build with this patch it uses -std=gnu++1z
Comment 13 Alex Christensen 2019-04-25 10:41:43 PDT
http://trac.webkit.org/r244653
Comment 14 Radar WebKit Bug Importer 2019-04-25 10:42:21 PDT
<rdar://problem/50210496>
Comment 15 Alex Christensen 2019-04-25 11:16:20 PDT
http://trac.webkit.org/r244654 :(
Comment 16 Alex Christensen 2019-04-25 11:49:39 PDT
http://trac.webkit.org/r244656
Comment 17 Alex Christensen 2019-04-25 13:08:26 PDT
https://trac.webkit.org/changeset/244662/webkit
Comment 18 Alex Christensen 2019-04-25 13:22:53 PDT
This also required the internal change ad34b33778bdc8b8f7644540ed72d8b11c51ab0e
Comment 19 Alex Christensen 2019-04-26 09:47:48 PDT
http://trac.webkit.org/r244692