Bug 199248

Summary: [MSVC] Catalog warnings
Product: WebKit Reporter: Don Olmstead <don.olmstead>
Component: CMakeAssignee: Don Olmstead <don.olmstead>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, bfulgham, ews-watchlist, gyuyoung.kim, Hironori.Fujii, mcatanzaro, mmaxfield, ryuan.choi, sergio
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
bfulgham: review+
Patch
none
Patch
none
Patch
none
Build fix
none
Build fix none

Description Don Olmstead 2019-06-26 17:12:37 PDT
Currently we have a big old list of warnings with no context. So its not clear what we're avoiding.
Comment 1 Don Olmstead 2019-06-26 17:28:37 PDT
Created attachment 372974 [details]
Patch
Comment 2 Brent Fulgham 2019-06-26 17:30:38 PDT
Comment on attachment 372974 [details]
Patch

Oh, nice! r=me (assuming ews is happy).
Comment 3 Fujii Hironori 2019-06-26 18:20:50 PDT
Comment on attachment 372974 [details]
Patch

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

> Source/cmake/OptionsMSVC.cmake:8
> +        /external:W2            # External header warning level

You should move below code for warning option here, and local variable _MSVC_WARNING_LEVEL.
    # More warnings. /W4 should be specified before -Wno-* options for clang-cl.
    string(REGEX REPLACE "/W3" "" CMAKE_C_FLAGS ${CMAKE_C_FLAGS})
    string(REGEX REPLACE "/W3" "" CMAKE_CXX_FLAGS ${CMAKE_CXX_FLAGS})
    WEBKIT_PREPEND_GLOBAL_COMPILER_FLAGS(/W4)
Comment 4 Fujii Hironori 2019-06-26 18:21:28 PDT
Comment on attachment 372974 [details]
Patch

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

>> Source/cmake/OptionsMSVC.cmake:8
>> +        /external:W2            # External header warning level
> 
> You should move below code for warning option here, and local variable _MSVC_WARNING_LEVEL.
>     # More warnings. /W4 should be specified before -Wno-* options for clang-cl.
>     string(REGEX REPLACE "/W3" "" CMAKE_C_FLAGS ${CMAKE_C_FLAGS})
>     string(REGEX REPLACE "/W3" "" CMAKE_CXX_FLAGS ${CMAKE_CXX_FLAGS})
>     WEBKIT_PREPEND_GLOBAL_COMPILER_FLAGS(/W4)

, and remove local variable _MSVC_WARNING_LEVEL.
Comment 5 Fujii Hironori 2019-06-27 20:39:17 PDT
Comment on attachment 372974 [details]
Patch

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

> Source/cmake/OptionsMSVC.cmake:137
>      string(REGEX REPLACE "/W3" "" CMAKE_CXX_FLAGS ${CMAKE_CXX_FLAGS})

BTW, it seems that we don't need to remove "/W3" anymore.
https://cmake.org/cmake/help/latest/release/3.15.html#id15
> With MSVC-like compilers the value of CMAKE_<LANG>_FLAGS no longer contains warning flags like /W3 by default. See policy CMP0092.
Comment 6 Don Olmstead 2019-06-28 12:16:15 PDT
Created attachment 373138 [details]
Patch
Comment 7 Don Olmstead 2021-02-05 16:14:04 PST
Created attachment 419482 [details]
Patch
Comment 8 Fujii Hironori 2021-02-05 17:58:12 PST
Comment on attachment 419482 [details]
Patch

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

> Source/cmake/OptionsMSVC.cmake:2
> +add_compile_options(/WX)

Don’t do this yet. I’m maintaining internal WinCairo clang jenkins jobs which report some compilation warnings.
Comment 9 Don Olmstead 2021-02-05 18:31:04 PST
Created attachment 419488 [details]
Patch
Comment 10 Don Olmstead 2021-02-05 18:44:39 PST
(In reply to Fujii Hironori from comment #8)
> Comment on attachment 419482 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=419482&action=review
> 
> > Source/cmake/OptionsMSVC.cmake:2
> > +add_compile_options(/WX)
> 
> Don’t do this yet. I’m maintaining internal WinCairo clang jenkins jobs
> which report some compilation warnings.

Patch now just catalogs all the warnings and removes ones I didn't hit with WinCairo.

🤞 that AppleWin comes back happy.
Comment 11 EWS 2021-02-05 21:09:52 PST
Committed r272457: <https://trac.webkit.org/changeset/272457>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 419488 [details].
Comment 12 Don Olmstead 2021-02-05 22:07:58 PST
Reopening for follow up with Apple Win.

EWS was saying patch wasn't relevant so modifying a file outside of Source/cmake to get things going.
Comment 13 Don Olmstead 2021-02-05 22:08:15 PST
Created attachment 419494 [details]
Build fix
Comment 14 Don Olmstead 2021-02-05 23:09:27 PST
Created attachment 419495 [details]
Build fix
Comment 15 EWS 2021-02-05 23:34:46 PST
Committed r272459: <https://trac.webkit.org/changeset/272459>

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