Move to C++14 Clang++ has been supporting C++14 for awhile. Our minimum versions for other compilers are GCC 4.9.0 and VS2015, which also support C++14. It's time to switch. If this task will take multiple passes, consider this an umbrella bug.
Attaching a WIP patch that is building fine on my Mac, just to get a feel of what other EWSs are doing.
Created attachment 279515 [details] Blind swipe at EWS
Note that there are important steps to take when updating ANGLE. See http://trac.webkit.org/wiki/UpdatingANGLE
Created attachment 279517 [details] Slightly updated just so I can get this at home this weekend
Created attachment 279537 [details] WIP Not too many changes needed to get things building on Mac, which this patch does. It also includes my first attempt at getting GCC happy (replacing cmake's c++11 option with c++1y, as the c++14 option didn't exist in our baseline gcc 4.9.0)
Attachment 279537 [details] did not pass style-queue: ERROR: Source/WebKit2/NetworkProcess/NetworkResourceLoadParameters.cpp:33: std::literals::chrono_literals is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.cpp:43: std::literals::chrono_literals is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/platform/graphics/cg/IOSurfacePool.cpp:36: std::literals::chrono_literals is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:48: std::literals::chrono_literals is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:52: std::literals::chrono_literals is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit2/WebProcess/Network/WebLoaderStrategy.cpp:59: std::literals::chrono_literals is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheSpeculativeLoadManager.cpp:43: std::literals::chrono_literals is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit2/UIProcess/ViewGestureController.cpp:38: std::literals::chrono_literals is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 8 in 25 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #6) > Attachment 279537 [details] did not pass style-queue: > > > ERROR: Source/WebKit2/NetworkProcess/NetworkResourceLoadParameters.cpp:33: > std::literals::chrono_literals is incorrectly named. Don't use underscores > in your identifier names. [readability/naming/underscores] [4] > ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.cpp:43: > std::literals::chrono_literals is incorrectly named. Don't use underscores > in your identifier names. [readability/naming/underscores] [4] > ERROR: Source/WebCore/platform/graphics/cg/IOSurfacePool.cpp:36: > std::literals::chrono_literals is incorrectly named. Don't use underscores > in your identifier names. [readability/naming/underscores] [4] > ERROR: Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:48: > std::literals::chrono_literals is incorrectly named. Don't use underscores > in your identifier names. [readability/naming/underscores] [4] > ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:52: > std::literals::chrono_literals is incorrectly named. Don't use underscores > in your identifier names. [readability/naming/underscores] [4] > ERROR: Source/WebKit2/WebProcess/Network/WebLoaderStrategy.cpp:59: > std::literals::chrono_literals is incorrectly named. Don't use underscores > in your identifier names. [readability/naming/underscores] [4] > ERROR: > Source/WebKit2/NetworkProcess/cache/NetworkCacheSpeculativeLoadManager.cpp: > 43: std::literals::chrono_literals is incorrectly named. Don't use > underscores in your identifier names. [readability/naming/underscores] [4] > ERROR: Source/WebKit2/UIProcess/ViewGestureController.cpp:38: > std::literals::chrono_literals is incorrectly named. Don't use underscores > in your identifier names. [readability/naming/underscores] [4] > Total errors found: 8 in 25 files > > > If any of these errors are false positives, please file a bug against > check-webkit-style. Yup. False positive. Filed https://bugs.webkit.org/show_bug.cgi?id=157969
I realize now that instead of sprinkling 'using namespace std::literals::chrono_literals' everywhere it's needed, it'll be better to change StdLibExtras.h to include <chrono> and then keep the 'using namespace...' I'll still let this patch run it's course on EWS before updating later.
Created attachment 279541 [details] Patch
Awesome :) looking forward to using some of the new things in C++14!
Comment on attachment 279541 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=279541&action=review This looks good to me. I'd prefer someone like Anders give the final green light. > Source/cmake/OptionsCommon.cmake:32 > + set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++1y") https://gcc.gnu.org/onlinedocs/gcc-4.9.3/gcc/C-Dialect-Options.html says this was experimental, and -std=c++14 does not exist. https://gcc.gnu.org/onlinedocs/gcc-5.2.0/gcc/C-Dialect-Options.html says this is deprecated in favor of -std=c++14. Linux folks should be aware of this.
(In reply to comment #11) > Comment on attachment 279541 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=279541&action=review > > This looks good to me. I'd prefer someone like Anders give the final green > light. > > > Source/cmake/OptionsCommon.cmake:32 > > + set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++1y") > > https://gcc.gnu.org/onlinedocs/gcc-4.9.3/gcc/C-Dialect-Options.html says > this was experimental, and -std=c++14 does not exist. > https://gcc.gnu.org/onlinedocs/gcc-5.2.0/gcc/C-Dialect-Options.html says > this is deprecated in favor of -std=c++14. Linux folks should be aware of > this. We have to use c++1y because c++14 doesn't exist in our bare minimum compiler of 4.9.0. The bots taught me this (yay, useful bots!) It should be fine; c++1y is automatically upgraded to c++14 on newer GCC where it's supported.
Comment on attachment 279541 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=279541&action=review Wasn't expecting the Linux bots to handle this. Cool. One possible problem: what about the Windows build? You changed Xcode, and CMake with GCC or Clang, but what about CMake with Visual Studio? >>> Source/cmake/OptionsCommon.cmake:32 >>> + set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++1y") >> >> https://gcc.gnu.org/onlinedocs/gcc-4.9.3/gcc/C-Dialect-Options.html says this was experimental, and -std=c++14 does not exist. >> https://gcc.gnu.org/onlinedocs/gcc-5.2.0/gcc/C-Dialect-Options.html says this is deprecated in favor of -std=c++14. Linux folks should be aware of this. > > We have to use c++1y because c++14 doesn't exist in our bare minimum compiler of 4.9.0. > > The bots taught me this (yay, useful bots!) > > It should be fine; c++1y is automatically upgraded to c++14 on newer GCC where it's supported. (In reply to comment #12) > We have to use c++1y because c++14 doesn't exist in our bare minimum > compiler of 4.9.0. Yes, this is fine. When GCC 4.9 was released, the standard had not been ratified yet and it could have turned into C++15 or something.
(In reply to comment #13) > Comment on attachment 279541 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=279541&action=review > > Wasn't expecting the Linux bots to handle this. Cool. > > One possible problem: what about the Windows build? You changed Xcode, and > CMake with GCC or Clang, but what about CMake with Visual Studio? That's a good question. I kinda figured OptionsCommon.cmake covered Windows, and kinda assumed I was right because Windows EWS built fine. I just confirmed that one of the changed files is included in the AppleWin build: Source/WebCore/platform/graphics/cg/IOSurfacePool.cpp It definitely uses C++14 today, and therefore since Windows EWS passed, I assumed VS2015 picked up the change somehow. But, the entire file is behind the "#if USE(IOSURFACE)" guard, and it's possible that's not enabled on windows. Alex is one of the windows cmake guru's and signed off... I'll make sure Brent takes a look, too.
(In reply to comment #14) > But, the entire file is behind the "#if USE(IOSURFACE)" guard, and it's > possible that's not enabled on windows. $ grep -ri IOSURFACE . | grep -v ChangeLog ... ./WebCore/PlatformAppleWin.cmake:add_definitions(-DQUARTZCORE_DLL -DDISABLE_COREIMAGE -DDISABLE_FRONTEND -DDISABLE_IOSURFACE -DDISABLE_RENDERSERVER So, indeed, it appears this code is not enabled on Windows.
In fact, Windows EWS even liked the very first patch, which shouldn't have worked for anybody. So it seems unlikely I've affected windows. Will wait until we get the word from an expert.
(In reply to comment #16) > In fact, Windows EWS even liked the very first patch, which shouldn't have > worked for anybody. > > So it seems unlikely I've affected windows. Or, at least, we have no actual way of knowing yet.
In email, Brent says this is fine. He also volunteered both himself and Alex to fix anything if that doesn't work out. :) But, to be nice, I'll land it tomorrow morning instead of late on a Saturday night.
(In reply to comment #18) > But, to be nice, I'll land it tomorrow morning instead of late on a Saturday > night. As you are well aware, since it is Saturday night, that automatically makes it all right.
(In reply to comment #19) > (In reply to comment #18) > > But, to be nice, I'll land it tomorrow morning instead of late on a Saturday > > night. > > As you are well aware, since it is Saturday night, that automatically makes > it all right. RIP, P.
(In reply to comment #20) > (In reply to comment #19) > > (In reply to comment #18) > > > But, to be nice, I'll land it tomorrow morning instead of late on a Saturday > > > night. > > > > As you are well aware, since it is Saturday night, that automatically makes > > it all right. > > RIP, P. :-(
Comment on attachment 279541 [details] Patch Clearing flags on attachment: 279541 Committed r201255: <http://trac.webkit.org/changeset/201255>
All reviewed patches have been landed. Closing bug.
This somehow broke the Yosemite build: https://build.webkit.org/builders/Apple%20Yosemite%20Release%20%28Build%29/builds/15433/steps/compile-webkit/logs/stdio In file included from /Volumes/Data/slave/yosemite-release/build/Source/JavaScriptCore/b3/B3MathExtras.cpp:32: /Volumes/Data/slave/yosemite-release/build/Source/JavaScriptCore/b3/B3CCallValue.h:42:23: error: no viable conversion from 'JSC::B3::Effects' to 'bool' Effects effects { Effects::forCall() }; /Volumes/Data/slave/yosemite-release/build/Source/JavaScriptCore/b3/B3CCallValue.h:42:42: error: missing field 'writes' initializer [-Werror,-Wmissing-field-initializers] Effects effects { Effects::forCall() }; Though it's not clear why/how.
(In reply to comment #24) > This somehow broke the Yosemite build: > https://build.webkit.org/builders/Apple%20Yosemite%20Release%20%28Build%29/ > builds/15433/steps/compile-webkit/logs/stdio > > > In file included from > /Volumes/Data/slave/yosemite-release/build/Source/JavaScriptCore/b3/ > B3MathExtras.cpp:32: > /Volumes/Data/slave/yosemite-release/build/Source/JavaScriptCore/b3/ > B3CCallValue.h:42:23: error: no viable conversion from 'JSC::B3::Effects' to > 'bool' > Effects effects { Effects::forCall() }; > /Volumes/Data/slave/yosemite-release/build/Source/JavaScriptCore/b3/ > B3CCallValue.h:42:42: error: missing field 'writes' initializer > [-Werror,-Wmissing-field-initializers] > Effects effects { Effects::forCall() }; > > Though it's not clear why/how. I'm not clear on the mechanism by which this ever worked - Effects is a struct without any obvious operator to bool.
Yosemite 32 bit built fine with r201255, which is where this patch landed. Yet 64 bits show this issue with r201256, which was a minimal followup. I'm perplexed.
(In reply to comment #24) > This somehow broke the Yosemite build: > https://build.webkit.org/builders/Apple%20Yosemite%20Release%20%28Build%29/ > builds/15433/steps/compile-webkit/logs/stdio > > > In file included from > /Volumes/Data/slave/yosemite-release/build/Source/JavaScriptCore/b3/ > B3MathExtras.cpp:32: > /Volumes/Data/slave/yosemite-release/build/Source/JavaScriptCore/b3/ > B3CCallValue.h:42:23: error: no viable conversion from 'JSC::B3::Effects' to > 'bool' > Effects effects { Effects::forCall() }; This doesn't make any sense, because I'm not sure where there's an attempt to use an Effects object as a bool. > /Volumes/Data/slave/yosemite-release/build/Source/JavaScriptCore/b3/ > B3CCallValue.h:42:42: error: missing field 'writes' initializer > [-Werror,-Wmissing-field-initializers] > Effects effects { Effects::forCall() }; This doesn't make any sense, because the Effects object created in Effects::forCall() is fully initialized, including in the writes member, which is a class with a default constructor!
Attempted Yosemite build fix suggested by Anders, landed in http://trac.webkit.org/changeset/201259
(In reply to comment #28) > Attempted Yosemite build fix suggested by Anders, landed in > http://trac.webkit.org/changeset/201259 Following stdio on that bot's current build attempt, it's definitely got past JSC, so that worked. I look forward to seeing it go green in a few dozen minutes.
(In reply to comment #13) > One possible problem: what about the Windows build? You changed Xcode, and > CMake with GCC or Clang, but what about CMake with Visual Studio? I think that just the fact that we require the use of VS2015 implies that we automatically get all the c++11 and c++14 support that VS2015 has. If I'm wrong, then I'll fix anything that comes up as a result of this.
Since Monday the 22nd I have this error when compiling WebKitGTK+ on Linux: error: ../../Source/WebKit2/NetworkProcess/cache/NetworkCacheSubresourcesEntry.cpp:64: error: undefined reference to 'WebKit::NetworkCache::Data::~Data()' No idea if it helps, but the problem may be related to this switch.
(In reply to comment #31) > Since Monday the 22nd I have this error when compiling WebKitGTK+ on Linux: > error: > ../../Source/WebKit2/NetworkProcess/cache/NetworkCacheSubresourcesEntry.cpp: > 64: error: undefined reference to 'WebKit::NetworkCache::Data::~Data()' > > No idea if it helps, but the problem may be related to this switch. Network cache speculative loading was very recently turned on on GTK port as well. I think this is why you are seeing this compilation error.
(In reply to comment #32) > (In reply to comment #31) > > Since Monday the 22nd I have this error when compiling WebKitGTK+ on Linux: > > error: > > ../../Source/WebKit2/NetworkProcess/cache/NetworkCacheSubresourcesEntry.cpp: > > 64: error: undefined reference to 'WebKit::NetworkCache::Data::~Data()' > > > > No idea if it helps, but the problem may be related to this switch. > > Network cache speculative loading was very recently turned on on GTK port as > well. I think this is why you are seeing this compilation error. See https://bugs.webkit.org/show_bug.cgi?id=157125