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, fujii.hironori, gyuyoung.kim, 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

Don Olmstead
Reported 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.
Attachments
Patch (9.68 KB, patch)
2019-06-26 17:28 PDT, Don Olmstead 		bfulgham: review+
DetailsFormatted DiffDiff
Patch (11.04 KB, patch)
2019-06-28 12:16 PDT, Don Olmstead 		no flags
DetailsFormatted DiffDiff
Patch (8.45 KB, patch)
2021-02-05 16:14 PST, Don Olmstead 		no flags
DetailsFormatted DiffDiff
Patch (7.58 KB, patch)
2021-02-05 18:31 PST, Don Olmstead 		no flags
DetailsFormatted DiffDiff
Build fix (1.31 KB, patch)
2021-02-05 22:08 PST, Don Olmstead 		no flags
DetailsFormatted DiffDiff
Build fix (1.25 KB, patch)
2021-02-05 23:09 PST, Don Olmstead 		no flags
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Don Olmstead
Comment 1 2019-06-26 17:28:37 PDT
Created attachment 372974 [details] Patch
Brent Fulgham
Comment 2 2019-06-26 17:30:38 PDT
Comment on attachment 372974 [details] Patch Oh, nice! r=me (assuming ews is happy).
Fujii Hironori
Comment 3 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)
Fujii Hironori
Comment 4 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.
Fujii Hironori
Comment 5 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.
Don Olmstead
Comment 6 2019-06-28 12:16:15 PDT
Created attachment 373138 [details] Patch
Don Olmstead
Comment 7 2021-02-05 16:14:04 PST
Created attachment 419482 [details] Patch
Fujii Hironori
Comment 8 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.
Don Olmstead
Comment 9 2021-02-05 18:31:04 PST
Created attachment 419488 [details] Patch
Don Olmstead
Comment 10 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.
EWS
Comment 11 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].
Don Olmstead
Comment 12 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.
Don Olmstead
Comment 13 2021-02-05 22:08:15 PST
Created attachment 419494 [details] Build fix
Don Olmstead
Comment 14 2021-02-05 23:09:27 PST
Created attachment 419495 [details] Build fix
EWS
Comment 15 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].
Note You need to log in before you can comment on or make changes to this bug.