Bug 140347 – Make 'TypeName' parameter unnecessary in CSSPropertyNames.in

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Chris Dumez]

Make 'TypeName' parameter unnecessary in CSSPropertyNames.in.

[Chris Dumez]

Created attachment 244431 [details]
Patch

[Darin Adler]

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.

[Anders Carlsson]

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

[Chris Dumez]

(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

[Chris Dumez]

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

[Anders Carlsson]

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

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 244480 [details]
Patch

[Chris Dumez]

Created attachment 244482 [details]
Patch

[Chris Dumez]

Created attachment 244486 [details]
Patch

[Chris Dumez]

Created attachment 244487 [details]
Patch

[Chris Dumez]

Comment on attachment 244487 [details]
Patch

green bubbles!

[Chris Dumez]

Comment on attachment 244487 [details]
Patch

Split the WTF change into https://bugs.webkit.org/show_bug.cgi?id=140401

[Chris Dumez]

Created attachment 244536 [details]
Patch

[Chris Dumez]

Comment on attachment 244536 [details]
Patch

The dependency has landed, the patch is ready for review.

[Sam Weinig]

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;
}

[Chris Dumez]

Created attachment 244559 [details]
Patch

[Chris Dumez]

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

[Darin Adler]

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.

[WebKit Commit Bot]

Comment on attachment 244559 [details]
Patch

Clearing flags on attachment: 244559

Committed r178434: <http://trac.webkit.org/changeset/178434>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

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

[Sam Weinig]

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