Bug 204443 - [MSVC] Add /experimental:newLambdaProcessor switch for better C++ conformance
Summary: [MSVC] Add /experimental:newLambdaProcessor switch for better C++ conformance
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CMake (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Fujii Hironori
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-11-20 22:27 PST by Fujii Hironori
Modified: 2019-12-03 03:28 PST (History)
16 users (show)

See Also:


Attachments
Patch (3.54 KB, patch)
2019-11-20 22:34 PST, Fujii Hironori
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fujii Hironori 2019-11-20 22:27:56 PST
[MSVC] Add /experimental:newLambdaProcessor switch for better C++ conformance

MSVC has a bug of lambda capture of 'this'. It has caused compilation errors repeatedly.

  Bug 204437 – [MSVC] error C2039: 'weakPtrFactory': is not a member of 'WebCore::DocumentStorageAccess::requestStorageAccess::<lambda_3f2cfd7704f93d8fe19d5b5f064f8add>'
  Bug 187035 – [Win] 'deref': is not a member of 'WebKit::WebResourceLoadStatisticsStore::updateCookiePartitioningForDomains::<lambda_9d761a6dc12d95db7fa2d6f3f5aa26fa>'
  Bug 184120 – [Win] MSVC can't compile WebResourceLoadStatisticsStore::scheduleClearInMemoryAndPersistent
  r251999

/experimental:newLambdaProcessor
https://docs.microsoft.com/en-us/cpp/overview/cpp-conformance-improvements?view=vs-2019#improvements_161
Comment 1 Fujii Hironori 2019-11-20 22:34:00 PST
Created attachment 384030 [details]
Patch
Comment 2 Fujii Hironori 2019-11-20 22:34:38 PST
I expect this change will break Apple Win7 bots because they are still using VS2017.
They should be shut down (Bug 197288 Comment 12).
Comment 3 Ross Kirsling 2019-11-21 08:33:14 PST
I've had to fix this in r250793, r240797, and r251999 as well.
Comment 4 Don Olmstead 2019-11-21 08:48:49 PST
(In reply to Ross Kirsling from comment #3)
> I've had to fix this in r250793, r240797, and r251999 as well.

Pretty sure the EWS caught all the compiler failures. Ignoring them is also a problem.
Comment 5 Konstantin Tokarev 2019-11-21 08:54:31 PST
Note that experimental lamba processor has its own conformance issues, e.g. https://developercommunity.visualstudio.com/content/problem/729465/experimentalnewlambdaprocessor-not-able-to-process.html, so if folks are ignoring EWS they still migth break MSVC with valid C++ code
Comment 6 Fujii Hironori 2019-11-24 18:00:48 PST
Using clang-cl seems an ideal solution (Bug 171618).
Seems no objection for the patch? I believe the patch reduce the maintenance for WebKit developers.
Anyone can review?
Comment 7 Fujii Hironori 2019-11-24 18:04:14 PST
(In reply to Ross Kirsling from comment #3)
> I've had to fix this in r250793, r240797, and r251999 as well.

IIUC, these changes don't need to be reverted in this patch.
Comment 8 Ross Kirsling 2019-11-25 09:34:20 PST
I don't object as long as the conformance is better, but I agree that having folks not ignore EWS is the most important regardless.
Comment 9 Fujii Hironori 2019-12-02 01:59:40 PST
review?
Comment 10 Don Olmstead 2019-12-02 10:05:35 PST
Are we really sure this is a good idea? I'd feel better if this was added to /permissive- https://docs.microsoft.com/en-us/cpp/build/reference/permissive-standards-conformance?view=vs-2019 or something along those lines
Comment 11 Fujii Hironori 2019-12-02 18:04:24 PST
Let's do that in: Bug 202842 – MSVC reports "wtf/RetainPtr.h(196): error C3861: 'CFAutorelease': identifier not found " with /permissive- on Windows
Comment 12 Fujii Hironori 2019-12-02 18:08:26 PST
Comment on attachment 384030 [details]
Patch

Clearing flags on attachment: 384030

Committed r253020: <https://trac.webkit.org/changeset/253020>
Comment 13 Fujii Hironori 2019-12-02 18:08:30 PST
All reviewed patches have been landed.  Closing bug.
Comment 14 Fujii Hironori 2019-12-02 18:25:08 PST
(In reply to Fujii Hironori from comment #2)
> I expect this change will break Apple Win7 bots because they are still using
> VS2017.
> They should be shut down (Bug 197288 Comment 12).

This change didn't actually break the Win7 bots, but a lot of warning messages are started reporting.

> cl : Command line warning D9002: ignoring unknown option '/experimental:newLambdaProcessor' [C:\cygwin\worker\win7-release-i386\build\WebKitBuild\Release\Tools\TestWebKitAPI\TestWTF.vcxproj]

https://build.webkit.org/builders/Apple%20Win%207%20Release%20%28Build%29/builds/8953

They should be shut down (Bug 197288 Comment 12).
Comment 15 Fujii Hironori 2019-12-03 03:24:07 PST
Oops. My patch breaks WinCairo bots which is using VS2019 16.0.
But, /experimental:newLambdaProcessor needs VS2019 16.1 or newer.

Don, can you upgrade your Docker image?
Comment 16 Fujii Hironori 2019-12-03 03:27:32 PST
Committed r253034: <https://trac.webkit.org/changeset/253034>
Comment 17 Fujii Hironori 2019-12-03 03:28:27 PST
Reverted in r253034. Reopened.