Bug 157948 – Move to C++14

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Brady Eidson]

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.

[Brady Eidson]

Attaching a WIP patch that is building fine on my Mac, just to get a feel of what other EWSs are doing.

[Brady Eidson]

Created attachment 279515 [details]
Blind swipe at EWS

[WebKit Commit Bot]

Note that there are important steps to take when updating ANGLE. See http://trac.webkit.org/wiki/UpdatingANGLE

[Brady Eidson]

Created attachment 279517 [details]
Slightly updated just so I can get this at home this weekend

[Brady Eidson]

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)

[WebKit Commit Bot]

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.

[Brady Eidson]

(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

[Brady Eidson]

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.

[Brady Eidson]

Created attachment 279541 [details]
Patch

[Chris Dumez]

Awesome :) looking forward to using some of the new things in C++14!

[Alex Christensen]

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.

[Brady Eidson]

(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.

[Michael Catanzaro]

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.

[Brady Eidson]

(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.

[Brady Eidson]

(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.

[Brady Eidson]

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.

[Brady Eidson]

(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.

[Brady Eidson]

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.

[Brent Fulgham]

(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.

[Brady Eidson]

(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.

[Brent Fulgham]

(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.

:-(

[WebKit Commit Bot]

Comment on attachment 279541 [details]
Patch

Clearing flags on attachment: 279541

Committed r201255: <http://trac.webkit.org/changeset/201255>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Brady Eidson]

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.

[Brady Eidson]

(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.

[Brady Eidson]

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.

[Brady Eidson]

(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!

[Brady Eidson]

Attempted Yosemite build fix suggested by Anders, landed in http://trac.webkit.org/changeset/201259

[Brady Eidson]

(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.

[Alex Christensen]

(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.

[Adrien Plazas]

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.

[Chris Dumez]

(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.

[Chris Dumez]

(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