Bug 157948

Summary: Move to C++14
Product: WebKit Reporter: Brady Eidson <beidson>
Component: WebKit Misc.Assignee: Brady Eidson <beidson>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, andersca, aplazas, benjamin, bfulgham, cdumez, cmarcelo, commit-queue, dino, graouts, keith_miller, kondapallykalyan, mark.lam, msaboff, saam
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Blind swipe at EWS
none
Slightly updated just so I can get this at home this weekend
none
WIP
none
Patch none

Description Brady Eidson 2016-05-20 10:07:29 PDT
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.
Comment 1 Brady Eidson 2016-05-20 16:43:10 PDT
Attaching a WIP patch that is building fine on my Mac, just to get a feel of what other EWSs are doing.
Comment 2 Brady Eidson 2016-05-20 16:43:31 PDT
Created attachment 279515 [details]
Blind swipe at EWS
Comment 3 WebKit Commit Bot 2016-05-20 16:44:53 PDT
Note that there are important steps to take when updating ANGLE. See http://trac.webkit.org/wiki/UpdatingANGLE
Comment 4 Brady Eidson 2016-05-20 17:04:31 PDT
Created attachment 279517 [details]
Slightly updated just so I can get this at home this weekend
Comment 5 Brady Eidson 2016-05-20 21:54:20 PDT
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)
Comment 6 WebKit Commit Bot 2016-05-20 21:55:11 PDT
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.
Comment 7 Brady Eidson 2016-05-20 21:59:50 PDT
(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
Comment 8 Brady Eidson 2016-05-20 22:20:03 PDT
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.
Comment 9 Brady Eidson 2016-05-21 08:12:02 PDT
Created attachment 279541 [details]
Patch
Comment 10 Chris Dumez 2016-05-21 10:33:10 PDT
Awesome :) looking forward to using some of the new things in C++14!
Comment 11 Alex Christensen 2016-05-21 17:40:02 PDT
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.
Comment 12 Brady Eidson 2016-05-21 18:39:50 PDT
(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 13 Michael Catanzaro 2016-05-21 21:00:18 PDT
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.
Comment 14 Brady Eidson 2016-05-21 21:27:30 PDT
(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.
Comment 15 Brady Eidson 2016-05-21 21:33:19 PDT
(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.
Comment 16 Brady Eidson 2016-05-21 21:34:42 PDT
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.
Comment 17 Brady Eidson 2016-05-21 21:35:52 PDT
(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.
Comment 18 Brady Eidson 2016-05-21 22:05:02 PDT
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.
Comment 19 Brent Fulgham 2016-05-21 22:07:28 PDT
(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.
Comment 20 Brady Eidson 2016-05-21 22:09:18 PDT
(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 21 Brent Fulgham 2016-05-21 22:10:09 PDT
(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 22 WebKit Commit Bot 2016-05-22 13:08:52 PDT
Comment on attachment 279541 [details]
Patch

Clearing flags on attachment: 279541

Committed r201255: <http://trac.webkit.org/changeset/201255>
Comment 23 WebKit Commit Bot 2016-05-22 13:08:59 PDT
All reviewed patches have been landed.  Closing bug.
Comment 24 Brady Eidson 2016-05-22 14:29:25 PDT
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.
Comment 25 Brady Eidson 2016-05-22 14:31:53 PDT
(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.
Comment 26 Brady Eidson 2016-05-22 14:34:39 PDT
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.
Comment 27 Brady Eidson 2016-05-22 15:02:08 PDT
(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!
Comment 28 Brady Eidson 2016-05-22 15:21:18 PDT
Attempted Yosemite build fix suggested by Anders, landed in http://trac.webkit.org/changeset/201259
Comment 29 Brady Eidson 2016-05-22 15:35:52 PDT
(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.
Comment 30 Alex Christensen 2016-05-23 10:38:04 PDT
(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.
Comment 31 Adrien Plazas 2016-05-24 06:38:20 PDT
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.
Comment 32 Chris Dumez 2016-05-24 07:23:22 PDT
(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.
Comment 33 Chris Dumez 2016-05-24 07:24:21 PDT
(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