Bug 140467 - Buildfix after r178434
Summary: Buildfix after r178434
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Csaba Osztrogonác
URL:
Keywords:
Depends on:
Blocks: 140347
  Show dependency treegraph
 
Reported: 2015-01-14 14:46 PST by Csaba Osztrogonác
Modified: 2015-01-14 15:33 PST (History)
3 users (show)

See Also:


Attachments
Patch (1.69 KB, patch)
2015-01-14 14:46 PST, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff
Patch (1.31 KB, patch)
2015-01-14 14:59 PST, Csaba Osztrogonác
cdumez: review+
cdumez: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Csaba Osztrogonác 2015-01-14 14:46:44 PST
Buildfix after r178434
Comment 1 Csaba Osztrogonác 2015-01-14 14:46:55 PST
Created attachment 244641 [details]
Patch
Comment 2 Chris Dumez 2015-01-14 14:49:38 PST
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.
Comment 3 Chris Dumez 2015-01-14 14:54:14 PST
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);" ?
Comment 4 Csaba Osztrogonác 2015-01-14 14:59:53 PST
Created attachment 244646 [details]
Patch
Comment 5 Csaba Osztrogonác 2015-01-14 15:00:47 PST
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.
Comment 6 Chris Dumez 2015-01-14 15:02:45 PST
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.
Comment 7 Chris Dumez 2015-01-14 15:04:36 PST
Comment on attachment 244646 [details]
Patch

r=me with Optional<float>() on left side.
Comment 8 Chris Dumez 2015-01-14 15:07:00 PST
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.
Comment 9 Csaba Osztrogonác 2015-01-14 15:08:29 PST
(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.
Comment 10 Csaba Osztrogonác 2015-01-14 15:10:10 PST
(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]"
Comment 11 Chris Dumez 2015-01-14 15:11:01 PST
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.
Comment 12 Chris Dumez 2015-01-14 15:11:45 PST
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
Comment 13 Chris Dumez 2015-01-14 15:14:36 PST
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).
Comment 14 Chris Dumez 2015-01-14 15:20:38 PST
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.
Comment 15 Csaba Osztrogonác 2015-01-14 15:21:05 PST
(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.
Comment 16 Darin Adler 2015-01-14 15:23:09 PST
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.
Comment 17 Chris Dumez 2015-01-14 15:29:31 PST
(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.
Comment 18 Csaba Osztrogonác 2015-01-14 15:33:01 PST
Fixed by https://trac.webkit.org/changeset/178454