Bug 140467 – Buildfix after r178434

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Csaba Osztrogonác]

Buildfix after r178434

[Csaba Osztrogonác]

Created attachment 244641 [details]
Patch

[Chris Dumez]

Comment on attachment 244641 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=244641&action=review

> Source/WebCore/css/StyleBuilderConverter.h:990
>          perspective = primitiveValue.computeLength<float>(styleResolver.state().cssToLengthConversionData());

This can potentially return a negative value. So that the one 2 lines below.

> Source/WebCore/css/StyleBuilderConverter.h:-996
> -    return perspective < 0 ? Nullopt : perspective;

you are changing behavior as we are no longer making sure the values returned by primitiveValue.computeLength() and primitiveValue.getDoubleValue() are positive.

[Chris Dumez]

Comment on attachment 244641 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=244641&action=review

> Source/WebCore/css/StyleBuilderConverter.h:996
> +    return perspective;

maybe just "return perspective < 0 ? Optional<float>() : Optional<float>(perspective);" ?

[Csaba Osztrogonác]

Created attachment 244646 [details]
Patch

[Csaba Osztrogonác]

Comment on attachment 244641 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=244641&action=review

>> Source/WebCore/css/StyleBuilderConverter.h:-996
>> -    return perspective < 0 ? Nullopt : perspective;
> 
> you are changing behavior as we are no longer making sure the values returned by primitiveValue.computeLength() and primitiveValue.getDoubleValue() are positive.

Good point, I thought it handles only the default -1 value.

>> Source/WebCore/css/StyleBuilderConverter.h:996
>> +    return perspective;
> 
> maybe just "return perspective < 0 ? Optional<float>() : Optional<float>(perspective);" ?

Nullopt is good, only the second one should be changed.

[Chris Dumez]

Comment on attachment 244646 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=244646&action=review

> Source/WebCore/css/StyleBuilderConverter.h:996
> +    return perspective < 0 ? Nullopt : Optional<float>(perspective);

We still have different types on each side. I am surprised this one works but my original code didn't. Even if this works, I still prefer to have a Optional<float>() on the left side for consistency.

[Chris Dumez]

Comment on attachment 244646 [details]
Patch

r=me with Optional<float>() on left side.

[Chris Dumez]

Comment on attachment 244646 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=244646&action=review

> Source/WebCore/ChangeLog:3
> +        Buildfix after r178434

Also, we might want to say EFL here as other ports built just fine I believe.

[Csaba Osztrogonác]

(In reply to comment #6)
> Comment on attachment 244646 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=244646&action=review
> 
> > Source/WebCore/css/StyleBuilderConverter.h:996
> > +    return perspective < 0 ? Nullopt : Optional<float>(perspective);
> 
> We still have different types on each side. I am surprised this one works
> but my original code didn't. Even if this works, I still prefer to have a
> Optional<float>() on the left side for consistency.

https://trac.webkit.org/browser/trunk/Source/WTF/wtf/Optional.h?rev=178372&order=name#L50

Optional(NulloptTag) implicit ctor is called, we don't call it
explicitly anywhere.

[Csaba Osztrogonác]

(In reply to comment #8)
> Comment on attachment 244646 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=244646&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        Buildfix after r178434
> 
> Also, we might want to say EFL here as other ports built just fine I believe.

It isn't related to EFL at all, but stricter GCC options:
"error: enumeral and non-enumeral type in conditional expression [-Werror]"

[Chris Dumez]

Comment on attachment 244646 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=244646&action=review

>>> Source/WebCore/css/StyleBuilderConverter.h:996
>>> +    return perspective < 0 ? Nullopt : Optional<float>(perspective);
>> 
>> We still have different types on each side. I am surprised this one works but my original code didn't. Even if this works, I still prefer to have a Optional<float>() on the left side for consistency.
> 
> https://trac.webkit.org/browser/trunk/Source/WTF/wtf/Optional.h?rev=178372&order=name#L50
> 
> Optional(NulloptTag) implicit ctor is called, we don't call it
> explicitly anywhere.

Well, then in my previous code, implicit Optional(const Length& value) was called and EFL had trouble building it. I don't see the difference.

[Chris Dumez]

Comment on attachment 244646 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=244646&action=review

>>>> Source/WebCore/css/StyleBuilderConverter.h:996
>>>> +    return perspective < 0 ? Nullopt : Optional<float>(perspective);
>>> 
>>> We still have different types on each side. I am surprised this one works but my original code didn't. Even if this works, I still prefer to have a Optional<float>() on the left side for consistency.
>> 
>> https://trac.webkit.org/browser/trunk/Source/WTF/wtf/Optional.h?rev=178372&order=name#L50
>> 
>> Optional(NulloptTag) implicit ctor is called, we don't call it
>> explicitly anywhere.
> 
> Well, then in my previous code, implicit Optional(const Length& value) was called and EFL had trouble building it. I don't see the difference.

https://trac.webkit.org/browser/trunk/Source/WTF/wtf/Optional.h?rev=178372&order=name#L55

[Chris Dumez]

Comment on attachment 244646 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=244646&action=review

>>>>> Source/WebCore/css/StyleBuilderConverter.h:996
>>>>> +    return perspective < 0 ? Nullopt : Optional<float>(perspective);
>>>> 
>>>> We still have different types on each side. I am surprised this one works but my original code didn't. Even if this works, I still prefer to have a Optional<float>() on the left side for consistency.
>>> 
>>> https://trac.webkit.org/browser/trunk/Source/WTF/wtf/Optional.h?rev=178372&order=name#L50
>>> 
>>> Optional(NulloptTag) implicit ctor is called, we don't call it
>>> explicitly anywhere.
>> 
>> Well, then in my previous code, implicit Optional(const Length& value) was called and EFL had trouble building it. I don't see the difference.
> 
> https://trac.webkit.org/browser/trunk/Source/WTF/wtf/Optional.h?rev=178372&order=name#L55

I am worried that with stricter build options your new code may not build either because we have different types on each side (even if it is sufficient for the current EFL build options).

[Chris Dumez]

Comment on attachment 244646 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=244646&action=review

>>> Source/WebCore/ChangeLog:3
>>> +        Buildfix after r178434
>> 
>> Also, we might want to say EFL here as other ports built just fine I believe.
> 
> It isn't related to EFL at all, but stricter GCC options:
> "error: enumeral and non-enumeral type in conditional expression [-Werror]"

Ok, fair enough.

[Csaba Osztrogonác]

(In reply to comment #13)
> Comment on attachment 244646 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=244646&action=review

> >> Well, then in my previous code, implicit Optional(const Length& value) was called and EFL had trouble building it. I don't see the difference.
> > 
> > https://trac.webkit.org/browser/trunk/Source/WTF/wtf/Optional.h?rev=178372&order=name#L55

It seems GCC doesn't try to call implicit Optional(const Length& value).
If it did, both size should have the same type.
 
> I am worried that with stricter build options your new code may not build
> either because we have different types on each side (even if it is
> sufficient for the current EFL build options).

Don't worry, I tried it, it builds. ;) I think GCC is clever enough to call 
the proper ctor for NullOptTag after seeing Optional<float>(perspective).
But it isn't able to call implicit ctor for both side in the same time.

[Darin Adler]

Comment on attachment 244646 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=244646&action=review

>>>>>> Source/WebCore/css/StyleBuilderConverter.h:996
>>>>>> +    return perspective < 0 ? Nullopt : Optional<float>(perspective);
>>>>> 
>>>>> We still have different types on each side. I am surprised this one works but my original code didn't. Even if this works, I still prefer to have a Optional<float>() on the left side for consistency.
>>>> 
>>>> https://trac.webkit.org/browser/trunk/Source/WTF/wtf/Optional.h?rev=178372&order=name#L50
>>>> 
>>>> Optional(NulloptTag) implicit ctor is called, we don't call it
>>>> explicitly anywhere.
>>> 
>>> Well, then in my previous code, implicit Optional(const Length& value) was called and EFL had trouble building it. I don't see the difference.
>> 
>> https://trac.webkit.org/browser/trunk/Source/WTF/wtf/Optional.h?rev=178372&order=name#L55
> 
> I am worried that with stricter build options your new code may not build either because we have different types on each side (even if it is sufficient for the current EFL build options).

I suggest we use an if statement instead of ? : to solve this problem.

[Chris Dumez]

(In reply to comment #16)
> Comment on attachment 244646 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=244646&action=review
> 
> >>>>>> Source/WebCore/css/StyleBuilderConverter.h:996
> >>>>>> +    return perspective < 0 ? Nullopt : Optional<float>(perspective);
> >>>>> 
> >>>>> We still have different types on each side. I am surprised this one works but my original code didn't. Even if this works, I still prefer to have a Optional<float>() on the left side for consistency.
> >>>> 
> >>>> https://trac.webkit.org/browser/trunk/Source/WTF/wtf/Optional.h?rev=178372&order=name#L50
> >>>> 
> >>>> Optional(NulloptTag) implicit ctor is called, we don't call it
> >>>> explicitly anywhere.
> >>> 
> >>> Well, then in my previous code, implicit Optional(const Length& value) was called and EFL had trouble building it. I don't see the difference.
> >> 
> >> https://trac.webkit.org/browser/trunk/Source/WTF/wtf/Optional.h?rev=178372&order=name#L55
> > 
> > I am worried that with stricter build options your new code may not build either because we have different types on each side (even if it is sufficient for the current EFL build options).
> 
> I suggest we use an if statement instead of ? : to solve this problem.

Sure, works for me.

[Csaba Osztrogonác]

Fixed by https://trac.webkit.org/changeset/178454