Bug 140347

Summary: Make 'TypeName' parameter unnecessary in CSSPropertyNames.in
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: CSSAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, commit-queue, darin, kling, koivisto, ossy, sam
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 140401, 140467    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Chris Dumez 2015-01-11 21:57:50 PST
Make 'TypeName' parameter unnecessary in CSSPropertyNames.in.
Comment 1 Chris Dumez 2015-01-11 22:11:04 PST
Created attachment 244431 [details]
Patch
Comment 2 Darin Adler 2015-01-12 08:20:31 PST
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.
Comment 3 Anders Carlsson 2015-01-12 08:28:54 PST
(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?
Comment 4 Chris Dumez 2015-01-12 09:52:13 PST
(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
Comment 5 Chris Dumez 2015-01-12 10:28:58 PST
(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.
Comment 6 Anders Carlsson 2015-01-12 10:33:15 PST
(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 7 Chris Dumez 2015-01-12 12:29:50 PST
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.
Comment 8 Chris Dumez 2015-01-12 17:21:33 PST
Created attachment 244480 [details]
Patch
Comment 9 Chris Dumez 2015-01-12 17:26:04 PST
Created attachment 244482 [details]
Patch
Comment 10 Chris Dumez 2015-01-12 18:52:00 PST
Created attachment 244486 [details]
Patch
Comment 11 Chris Dumez 2015-01-12 18:56:52 PST
Created attachment 244487 [details]
Patch
Comment 12 Chris Dumez 2015-01-12 19:56:15 PST
Comment on attachment 244487 [details]
Patch

green bubbles!
Comment 13 Chris Dumez 2015-01-13 11:57:51 PST
Comment on attachment 244487 [details]
Patch

Split the WTF change into https://bugs.webkit.org/show_bug.cgi?id=140401
Comment 14 Chris Dumez 2015-01-13 12:41:46 PST
Created attachment 244536 [details]
Patch
Comment 15 Chris Dumez 2015-01-13 13:59:29 PST
Comment on attachment 244536 [details]
Patch

The dependency has landed, the patch is ready for review.
Comment 16 Sam Weinig 2015-01-13 15:19:19 PST
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;
}
Comment 17 Chris Dumez 2015-01-13 15:58:34 PST
Created attachment 244559 [details]
Patch
Comment 18 Chris Dumez 2015-01-13 15:58:58 PST
(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 19 Darin Adler 2015-01-14 11:31:28 PST
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 20 WebKit Commit Bot 2015-01-14 12:11:43 PST
Comment on attachment 244559 [details]
Patch

Clearing flags on attachment: 244559

Committed r178434: <http://trac.webkit.org/changeset/178434>
Comment 21 WebKit Commit Bot 2015-01-14 12:11:52 PST
All reviewed patches have been landed.  Closing bug.
Comment 22 Csaba Osztrogonác 2015-01-14 14:47:12 PST
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
Comment 23 Sam Weinig 2015-01-15 12:54:30 PST
(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.