Re: [2/4] git commit: [flex-asjs] [refs/heads/release0.8.0] - looks like we get '' (empty string) sometimes which coerces to 0. We want it to be NaN

classic Classic list List threaded Threaded
14 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: [2/4] git commit: [flex-asjs] [refs/heads/release0.8.0] - looks like we get '' (empty string) sometimes which coerces to 0. We want it to be NaN

Justin Mclean
Administrator
Hi,

This is probably a simpler / less costly way of doing the same thing:

var pixels:Number = NaN;
var strpixels:String = element.style.width as String;
if (strpixels.indexOf('%') === -1)
    pixels = parseFloat(strpixels);

Justin

Reply | Threaded
Open this post in threaded view
|

Re: [2/4] git commit: [flex-asjs] [refs/heads/release0.8.0] - looks like we get '' (empty string) sometimes which coerces to 0. We want it to be NaN

Justin Mclean
Administrator
Hi,

in fact this code will throw a RTE is width / strpixels is null so you may want to reconsider the implementation.

Justin
Reply | Threaded
Open this post in threaded view
|

Re: [2/4] git commit: [flex-asjs] [refs/heads/release0.8.0] - looks like we get '' (empty string) sometimes which coerces to 0. We want it to be NaN

Justin Mclean
Administrator
Hi,

In more detail the issue is that the property is likely to default to undefined and casting that to String in JS like so org.apache.flex.utils.Language.as(undefined, String)) will set the value to null and strpixels.length with throw a RTE. So it’s likely this change in will cause all sort of issues.

Thanks,
Justin
Reply | Threaded
Open this post in threaded view
|

Re: [2/4] git commit: [flex-asjs] [refs/heads/release0.8.0] - looks like we get '' (empty string) sometimes which coerces to 0. We want it to be NaN

Harbs
Language.as() is not being called (@flexjsignorecoercion is being used).

> On Jun 6, 2017, at 12:50 PM, Justin Mclean <[hidden email]> wrote:
>
> Hi,
>
> In more detail the issue is that the property is likely to default to undefined and casting that to String in JS like so org.apache.flex.utils.Language.as(undefined, String)) will set the value to null and strpixels.length with throw a RTE. So it’s likely this change in will cause all sort of issues.
>
> Thanks,
> Justin

Reply | Threaded
Open this post in threaded view
|

Re: [2/4] git commit: [flex-asjs] [refs/heads/release0.8.0] - looks like we get '' (empty string) sometimes which coerces to 0. We want it to be NaN

Justin Mclean
Administrator
Hi,

> Language.as() is not being called (@flexjsignorecoercion is being used).

And it will still throw an RTE if strpixels is null.

>  if (strpixels !== null && strpixels.indexOf('%') != -1)

will be false

> else if (strpixels.length == 0)

throws RTE

Thanks,
Justin
Reply | Threaded
Open this post in threaded view
|

Re: [2/4] git commit: [flex-asjs] [refs/heads/release0.8.0] - looks like we get '' (empty string) sometimes which coerces to 0. We want it to be NaN

piotrz
In reply to this post by Justin Mclean
Justin,

You propose changing whole this part [1] with yours ?

[1] https://paste.apache.org/L1Mq

Just wanted to understand.

Thanks,
Piotr
Apache Flex PMC
piotrzarzycki21@gmail.com
Reply | Threaded
Open this post in threaded view
|

Re: [2/4] git commit: [flex-asjs] [refs/heads/release0.8.0] - looks like we get '' (empty string) sometimes which coerces to 0. We want it to be NaN

Justin Mclean
Administrator
Hi,

> You propose changing whole this part [1] with yours ?

Or something similar / along those line that was off the top of my head and had not been tested.

Perhaps if null is never passed in then it doesn’t need to be checked for and the code may not RTE?

Thanks,
Justin
Reply | Threaded
Open this post in threaded view
|

Re: [2/4] git commit: [flex-asjs] [refs/heads/release0.8.0] - looks like we get '' (empty string) sometimes which coerces to 0. We want it to be NaN

piotrz
In reply to this post by Justin Mclean
I think it would be good to change all those with your proposition. For me it's cleaner, but with this one from you:

var strpixels:String = element.style.width as String;
if (strpixels.indexOf('%') === -1)

We may have still RTE ?

Piotr
Apache Flex PMC
piotrzarzycki21@gmail.com
Reply | Threaded
Open this post in threaded view
|

Re: [2/4] git commit: [flex-asjs] [refs/heads/release0.8.0] - looks like we get '' (empty string) sometimes which coerces to 0. We want it to be NaN

Justin Mclean
Administrator
Hi,

> var strpixels:String = element.style.width as String;
> if (strpixels.indexOf('%') === -1)
>
> We may have still RTE ?

Yep it would also RTE if strpixels was null.

So perhaps this instead?
> if (strpixels !== null && strpixels.indexOf('%') === -1)


Justin
Reply | Threaded
Open this post in threaded view
|

Re: [2/4] git commit: [flex-asjs] [refs/heads/release0.8.0] - looks like we get '' (empty string) sometimes which coerces to 0. We want it to be NaN

piotrz
I think it would be great! Let see what Alex thing.

Piotr
Apache Flex PMC
piotrzarzycki21@gmail.com
Reply | Threaded
Open this post in threaded view
|

Re: [2/4] git commit: [flex-asjs] [refs/heads/release0.8.0] - looks like we get '' (empty string) sometimes which coerces to 0. We want it to be NaN

Justin Mclean
Administrator
In reply to this post by piotrz
Hi,

Just profiled these with a variety of inputs and the first is around 3 times as fast as the second.

I added a null check to stop the RTE, if the null check is not needed it can be removed from both.

public function test(str:String):Number {
    var pixels:Number = NaN;
    if (str !== null && str.indexOf('%') === -1)
        pixels = parseFloat(str);

    return pixels;  
}

public function test2(str:String):Number {
    var pixels:Number;
    if (str !== null && str.indexOf('%') != -1)
        pixels = NaN;
    else if (str !== null && str.length == 0)
        pixels = NaN;
    else
        pixels = parseFloat(str);

    return pixels;
}

Thanks,
Justin

Reply | Threaded
Open this post in threaded view
|

Re: [2/4] git commit: [flex-asjs] [refs/heads/release0.8.0] - looks like we get '' (empty string) sometimes which coerces to 0. We want it to be NaN

Harbs
In reply to this post by Justin Mclean
True.

else if (strpixels.length == 0)

should probably be:
else if (strpixels === “”)

> On Jun 6, 2017, at 1:01 PM, Justin Mclean <[hidden email]> wrote:
>
> Hi,
>
>> Language.as() is not being called (@flexjsignorecoercion is being used).
>
> And it will still throw an RTE if strpixels is null.
>
>> if (strpixels !== null && strpixels.indexOf('%') != -1)
>
> will be false
>
>> else if (strpixels.length == 0)
>
> throws RTE
>
> Thanks,
> Justin

Reply | Threaded
Open this post in threaded view
|

Re: [2/4] git commit: [flex-asjs] [refs/heads/release0.8.0] - looks like we get '' (empty string) sometimes which coerces to 0. We want it to be NaN

Harbs
Changed.

> On Jun 6, 2017, at 1:37 PM, Harbs <[hidden email]> wrote:
>
> True.
>
> else if (strpixels.length == 0)
>
> should probably be:
> else if (strpixels === “”)
>
>> On Jun 6, 2017, at 1:01 PM, Justin Mclean <[hidden email]> wrote:
>>
>> Hi,
>>
>>> Language.as() is not being called (@flexjsignorecoercion is being used).
>>
>> And it will still throw an RTE if strpixels is null.
>>
>>> if (strpixels !== null && strpixels.indexOf('%') != -1)
>>
>> will be false
>>
>>> else if (strpixels.length == 0)
>>
>> throws RTE
>>
>> Thanks,
>> Justin
>

Reply | Threaded
Open this post in threaded view
|

Re: [2/4] git commit: [flex-asjs] [refs/heads/release0.8.0] - looks like we get '' (empty string) sometimes which coerces to 0. We want it to be NaN

Harbs
In reply to this post by Justin Mclean
It sounds like a micro-optimization, but it saves some bytes and doesn’t look like a functional change, so go knock yourself out… ;-)

> On Jun 6, 2017, at 1:43 PM, Justin Mclean <[hidden email]> wrote:
>
> Hi,
>
> Just profiled these with a variety of inputs and the first is around 3 times as fast as the second.
>
> I added a null check to stop the RTE, if the null check is not needed it can be removed from both.
>
> public function test(str:String):Number {
>    var pixels:Number = NaN;
>    if (str !== null && str.indexOf('%') === -1)
>        pixels = parseFloat(str);
>
>    return pixels;  
> }
>
> public function test2(str:String):Number {
>    var pixels:Number;
>    if (str !== null && str.indexOf('%') != -1)
>        pixels = NaN;
>    else if (str !== null && str.length == 0)
>        pixels = NaN;
>    else
>        pixels = parseFloat(str);
>
>    return pixels;
> }
>
> Thanks,
> Justin
>