| Summary: | Buildfix after r178434 | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Csaba Osztrogonác <ossy> | ||||||
| Component: | New Bugs | Assignee: | Csaba Osztrogonác <ossy> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | cdumez, darin, ossy | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Bug Depends on: | |||||||||
| Bug Blocks: | 140347 | ||||||||
| Attachments: |
|
||||||||
|
Description
Csaba Osztrogonác
2015-01-14 14:46:44 PST
Created attachment 244641 [details]
Patch
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 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);" ? Created attachment 244646 [details]
Patch
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 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 on attachment 244646 [details]
Patch
r=me with Optional<float>() on left side.
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. (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. (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 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 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 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 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. (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 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. (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. |