Make 'TypeName' parameter unnecessary in CSSPropertyNames.in.
Created attachment 244431 [details] Patch
Comment on attachment 244431 [details] Patch Here are the Windows build errors: 1>C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\include\private\wtf/Optional.h(176): error C2621: 'WTF::Optional<WebCore::GridTrackSize>::m_value' : illegal union member; type 'WebCore::GridTrackSize' has a copy constructor (C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\obj32\WebCore\DerivedSources\StyleBuilder.cpp) C:\cygwin\home\buildbot\WebKit\Source\WebCore\css\StyleBuilderConverter.h(911) : see reference to class template instantiation 'WTF::Optional<WebCore::GridTrackSize>' being compiled 1>C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\include\private\wtf/Optional.h(177): warning C4408: anonymous union did not declare any data members (C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\obj32\WebCore\DerivedSources\StyleBuilder.cpp) 1>C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\include\private\wtf/Optional.h(176): error C2621: 'WTF::Optional<WebCore::GridPosition>::m_value' : illegal union member; type 'WebCore::GridPosition' has a copy constructor (C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\obj32\WebCore\DerivedSources\StyleBuilder.cpp) C:\cygwin\home\buildbot\WebKit\Source\WebCore\css\StyleBuilderConverter.h(920) : see reference to class template instantiation 'WTF::Optional<WebCore::GridPosition>' being compiled 1>C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\include\private\wtf/Optional.h(176): error C2621: 'WTF::Optional<WebCore::Length>::m_value' : illegal union member; type 'WebCore::Length' has a copy constructor (C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\obj32\WebCore\DerivedSources\StyleBuilder.cpp) C:\cygwin\home\buildbot\WebKit\Source\WebCore\css\StyleBuilderConverter.h(969) : see reference to class template instantiation 'WTF::Optional<WebCore::Length>' being compiled 1>C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\include\private\wtf/Optional.h(176): error C2621: 'WTF::Optional<WebCore::FilterOperations>::m_value' : illegal union member; type 'WebCore::FilterOperations' has a copy constructor (C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\obj32\WebCore\DerivedSources\StyleBuilder.cpp) C:\cygwin\home\buildbot\WebKit\Source\WebCore\css\StyleBuilderConverter.h(1033) : see reference to class template instantiation 'WTF::Optional<WebCore::FilterOperations>' being compiled Looks like WTF::Optional doesn’t work well with the Visual Studio compiler. I wonder how this can be fixed.
(In reply to comment #2) > > Looks like WTF::Optional doesn’t work well with the Visual Studio compiler. > I wonder how this can be fixed. Looks like it's because VS doesn't support unrestricted unions?
(In reply to comment #3) > (In reply to comment #2) > > > > Looks like WTF::Optional doesn’t work well with the Visual Studio compiler. > > I wonder how this can be fixed. > > Looks like it's because VS doesn't support unrestricted unions? Indeed, unrestricted union are still not supported by Visual studio according to: http://msdn.microsoft.com/en-us/library/hh567368.aspx
(In reply to comment #4) > (In reply to comment #3) > > (In reply to comment #2) > > > > > > Looks like WTF::Optional doesn’t work well with the Visual Studio compiler. > > > I wonder how this can be fixed. > > > > Looks like it's because VS doesn't support unrestricted unions? > > Indeed, unrestricted union are still not supported by Visual studio > according to: > http://msdn.microsoft.com/en-us/library/hh567368.aspx Maybe we can use std::aligned_storage? This seems to be what boost is using for boost::optional.
(In reply to comment #5) > (In reply to comment #4) > > (In reply to comment #3) > > > (In reply to comment #2) > > > > > > > > Looks like WTF::Optional doesn’t work well with the Visual Studio compiler. > > > > I wonder how this can be fixed. > > > > > > Looks like it's because VS doesn't support unrestricted unions? > > > > Indeed, unrestricted union are still not supported by Visual studio > > according to: > > http://msdn.microsoft.com/en-us/library/hh567368.aspx > > Maybe we can use std::aligned_storage? This seems to be what boost is using > for boost::optional. Yup, aligned_storage + placement new, just like NeverDestroyed does.
Comment on attachment 244431 [details] Patch I am working on fixing the MSVC build by using aligned_storage + placement new, instead of an unrestricted union in Optional.h.
Created attachment 244480 [details] Patch
Created attachment 244482 [details] Patch
Created attachment 244486 [details] Patch
Created attachment 244487 [details] Patch
Comment on attachment 244487 [details] Patch green bubbles!
Comment on attachment 244487 [details] Patch Split the WTF change into https://bugs.webkit.org/show_bug.cgi?id=140401
Created attachment 244536 [details] Patch
Comment on attachment 244536 [details] Patch The dependency has landed, the patch is ready for review.
Comment on attachment 244536 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=244536&action=review > Source/WebCore/css/StyleBuilderConverter.h:916 > + Optional<GridTrackSize> optionalTrackSize; > + GridTrackSize trackSize; > + if (createGridTrackSize(value, trackSize, styleResolver)) > + optionalTrackSize = trackSize; > + return optionalTrackSize; I prefer to structure this type of thing like: { GridTrackSize trackSize; if (createGridTrackSize(value, trackSize, styleResolver)) return trackSize; return Nullopt; }
Created attachment 244559 [details] Patch
(In reply to comment #16) > Comment on attachment 244536 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=244536&action=review > > > Source/WebCore/css/StyleBuilderConverter.h:916 > > + Optional<GridTrackSize> optionalTrackSize; > > + GridTrackSize trackSize; > > + if (createGridTrackSize(value, trackSize, styleResolver)) > > + optionalTrackSize = trackSize; > > + return optionalTrackSize; > > I prefer to structure this type of thing like: > { > GridTrackSize trackSize; > if (createGridTrackSize(value, trackSize, styleResolver)) > return trackSize; > return Nullopt; > } Done.
Comment on attachment 244559 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=244559&action=review > Source/WebCore/css/StyleBuilderConverter.h:1006 > marqueeLength = Length(1, Fixed); // 1px. > - return true; > + break; I think these would read better as return statements instead of assignment with a break; it’s sort of the switch version of our “early return”. Might be nice to come back and clean that up even after landing this patch. > Source/WebCore/css/StyleBuilderConverter.h:1030 > + if (styleResolver.createFilterOperations(value, operations)) > + return operations; > + return Nullopt; I know Sam asked you to write it this way. I like it the opposite way with the early return for the error case. if (!x) return Nullopt. Others have told me this is not good for optimizers that like to predict that branches *are* taken, but I like the pattern where the success path and main logic is the main flow of the function and the failures or optimized cases are earlier drains.
Comment on attachment 244559 [details] Patch Clearing flags on attachment: 244559 Committed r178434: <http://trac.webkit.org/changeset/178434>
All reviewed patches have been landed. Closing bug.
Comment on attachment 244559 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=244559&action=review > Source/WebCore/css/StyleBuilderConverter.h:996 > + return perspective < 0 ? Nullopt : perspective; It broke the EFL build, but unfortunately you didn't have any chance to catch it before landing, because EFL EWS is offline long time ago. :( In file included from DerivedSources/WebCore/StyleBuilder.cpp:9:0: ../../Source/WebCore/css/StyleBuilderConverter.h: In static member function ‘static WTF::Optional<float> WebCore::StyleBuilderConverter::convertPerspective(WebCore::StyleResolver&, WebCore::CSSValue&)’: ../../Source/WebCore/css/StyleBuilderConverter.h:996:40: error: enumeral and non-enumeral type in conditional expression [-Werror] return perspective < 0 ? Nullopt : perspective; ^ cc1plus: all warnings being treated as errors I filed a new bug report to fix it: bug140467
(In reply to comment #19) > Comment on attachment 244559 [details] > > > Source/WebCore/css/StyleBuilderConverter.h:1030 > > + if (styleResolver.createFilterOperations(value, operations)) > > + return operations; > > + return Nullopt; > > I know Sam asked you to write it this way. I like it the opposite way with > the early return for the error case. if (!x) return Nullopt. Others have > told me this is not good for optimizers that like to predict that branches > *are* taken, but I like the pattern where the success path and main logic is > the main flow of the function and the failures or optimized cases are > earlier drains. The part I was really trying to comment on was the direct returning of Nullopt and the value, and avoiding the Optional<> local. I also tend to prefer the early return for the error case.