You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@flex.apache.org by Justin Mclean <ju...@classsoftware.com> on 2017/03/14 23:01:50 UTC
Typedef issue causing flex-asjs to fail to compile
Hi,
There also looks to be a typedef issue. Current CCSUtils is failing to compile:
/Users/justinmclean/flex-asjs/frameworks/projects/Core/src/main/flex/org/apache/flex/utils/CSSUtils.as(52): col: 27 Incorrect number of arguments. Expected 2
rgba[3] = parseInt(""+(rgba[3]/255)*1000) / 1000;
^
The typedef has this comment which seems a clue:
/**
* Parse an integer. Use of {@code parseInt} without {@code base} is strictly
* banned in Google. If you really want to parse octal or hex based on the
* leader, then pass {@code undefined} as the base.
*
Thanks,
Justin
Re: Typedef issue causing flex-asjs to fail to compile
Posted by Justin Mclean <ju...@classsoftware.com>.
HI,
> I agree that 10 is wrong
I assume you mean as a default?
Thanks,
Justin
Re: Typedef issue causing flex-asjs to fail to compile
Posted by Alex Harui <ah...@adobe.com>.
OK. I'll take a look at the failing test later.
Thanks again for working on this.
-Alex
On 3/22/17, 9:40 AM, "Greg Dove" <gr...@gmail.com> wrote:
>Alex, I pushed this change, using undefined as discussed in the ticket.
>The standard compiler tests all pass, but there is, I think, an unrelated
>test
>in flexjs.dependent.tests that is currently failing
>in TestFlexJSMXMLApplication (I admit that I did not revert my change to
>check it was occurring prior, but I can't see how it could be related)
>It is a css in/css out comparison test.
>As near as I can tell the failing css test had the correct css settings in
>the output, but the order of them does not always match the expected
>output
>(it does for the most part). So height:176px might appear before color:#ff
>in expected result in some parts and occurs the other way around in the
>corresponding part of the output, but the expected items are all there. I
>did not dig into this, but I perhaps the ordering was changed or some new
>functionality was added where perhaps ordering of output was not
>guaranteed.
>
>
>
>
>On Tue, Mar 21, 2017 at 3:56 AM, Alex Harui <ah...@adobe.com> wrote:
>
>>
>>
>> On 3/20/17, 12:00 AM, "Greg Dove" <gr...@gmail.com> wrote:
>>
>> >I agree that 10 is wrong, but
>> >reading both AS and JS doc, I'm now thinking that if the second
>>argument
>> >is missing that the compiler should use 0.
>>
>> +1. Let's use 0.
>>
>> Thanks for doing such a through investigation.
>> -Alex
>>
>>
Re: Typedef issue causing flex-asjs to fail to compile
Posted by Greg Dove <gr...@gmail.com>.
Alex, I pushed this change, using undefined as discussed in the ticket.
The standard compiler tests all pass, but there is, I think, an unrelated test
in flexjs.dependent.tests that is currently failing
in TestFlexJSMXMLApplication (I admit that I did not revert my change to
check it was occurring prior, but I can't see how it could be related)
It is a css in/css out comparison test.
As near as I can tell the failing css test had the correct css settings in
the output, but the order of them does not always match the expected output
(it does for the most part). So height:176px might appear before color:#ff
in expected result in some parts and occurs the other way around in the
corresponding part of the output, but the expected items are all there. I
did not dig into this, but I perhaps the ordering was changed or some new
functionality was added where perhaps ordering of output was not
guaranteed.
On Tue, Mar 21, 2017 at 3:56 AM, Alex Harui <ah...@adobe.com> wrote:
>
>
> On 3/20/17, 12:00 AM, "Greg Dove" <gr...@gmail.com> wrote:
>
> >I agree that 10 is wrong, but
> >reading both AS and JS doc, I'm now thinking that if the second argument
> >is missing that the compiler should use 0.
>
> +1. Let's use 0.
>
> Thanks for doing such a through investigation.
> -Alex
>
>
Re: Typedef issue causing flex-asjs to fail to compile
Posted by Alex Harui <ah...@adobe.com>.
On 3/20/17, 12:00 AM, "Greg Dove" <gr...@gmail.com> wrote:
>I agree that 10 is wrong, but
>reading both AS and JS doc, I'm now thinking that if the second argument
>is missing that the compiler should use 0.
+1. Let's use 0.
Thanks for doing such a through investigation.
-Alex
Re: Typedef issue causing flex-asjs to fail to compile
Posted by Greg Dove <gr...@gmail.com>.
I agree that 10 is wrong, but
reading both AS and JS doc, I'm now thinking that if the second argument
is missing that the compiler should use 0.
Please see the note in the comment of this ticket[1]:
We need to decide whether we should add undefined as the second param (it
seems this is the preferred option by GCC instead of zero, I am not sure
why other than that they specify it) or possibly use the GCL version of
parseInt in these cases for full backwards compatibility with older
browsers.
What are your thoughts about this?
1.
https://issues.apache.org/jira/browse/FLEX-35283?focusedCommentId=15932208&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15932208
Re: Typedef issue causing flex-asjs to fail to compile
Posted by Alex Harui <ah...@adobe.com>.
On 3/14/17, 11:17 PM, "Greg Dove" <gr...@gmail.com> wrote:
>to clarify :
>
>I got the typedefs building eventually and compiling the single arg
>signature for partseInt is fine, but the compiler makes a fixed assumption
>for 10 radix for any string and adds that second param instead of allowing
>the native implementation to do its thing.
OK, good to know your typedefs are good. I agree that 10 is wrong, but
reading both AS and JS doc, I'm now thinking that if the second argument
is missing that the compiler should use 0.
>
>If there are string formats like octal that get inferred differently from
>the string in js but not in swf then that complicates things but if they
>are identical (and if there is no requirement to have the js output *not*
>have an optional argument) then the simplest thing of course is to pass it
>through unchanged.
>
>I will check the various browsers and permutations this weekend to see
>what
>happens. I already added a ticket for me
Thanks for digging into it.
-Alex
Re: Typedef issue causing flex-asjs to fail to compile
Posted by Greg Dove <gr...@gmail.com>.
to clarify :
I got the typedefs building eventually and compiling the single arg
signature for partseInt is fine, but the compiler makes a fixed assumption
for 10 radix for any string and adds that second param instead of allowing
the native implementation to do its thing.
If there are string formats like octal that get inferred differently from
the string in js but not in swf then that complicates things but if they
are identical (and if there is no requirement to have the js output *not*
have an optional argument) then the simplest thing of course is to pass it
through unchanged.
I will check the various browsers and permutations this weekend to see what
happens. I already added a ticket for me
On Wed, Mar 15, 2017 at 7:03 PM, Alex Harui <ah...@adobe.com> wrote:
>
>
> On 3/14/17, 10:42 PM, "Greg Dove" <gr...@gmail.com> wrote:
>
> >On Wed, Mar 15, 2017 at 6:33 PM, Alex Harui <ah...@adobe.com> wrote:
> >
> >> Here's a better link to the old thread. Looks like I did mess around
> >>with
> >> parseInt earlier.
> >>
> >> https://s.apache.org/rHq2
> >>
> >>
> >> The goal is to match what AS does and get the performance benefit you
> >> measured and maybe prevent browsers from doing something strange.
> >>
> >> Hopefully it won't require sniffing the first parameter ourselves.
> >>
> >> Thanks,
> >> -Alex
> >>
> >>
> >>
> >Thanks for that link, the original was not working for me.
> >In theory it should simply take the string and deduce the radix internally
> >depending on whether the string has '0x' at the start or not. That's why
> >specifying the radix should always be faster.
> >But we cannot assume it is 10 for all strings in a compiler generated
> >version with the (apparently) mandatory second param for GCL.
> >
> >So it is a bit of challenge if we want to keep the AS behaviour, and if
> >the
> >GCL lib does not like having the second arg missing.
> >Basically for perfomance you need to specify the second param in the
> >native
> >version and that obviously will always be the better option.
> >I think the compiler generated version will need to do some sniffing
> >unfortunately, (if we are to keep behaviour) and will make that
> >performance
> >difference even more true, but will give it a bit more thought.
> >.
>
> I don't think there is any code at runtime that requires the second
> argument. We patch the files in the JS.swc so the second argument is
> optional. AFAIK, in the actual browser, the second parameter is optional.
> IMO, what you see in the original externs is Google's policy for their
> developers, not a runtime API spec. But you can see in the old thread
> that MDN says you really "should" set a second parameter.
>
> Somehow, that parseInt patch didn't work for Justin and you. So that's
> the first problem. Your flex-typedefs/js build should generate a
> parseInt.as with an optional second argument because the es3.js is patched
> to have an optional second argument.
>
> Then we can discuss whether the compiler should set 10 or 0 or trust the
> modern browsers to not think Octal or be unpredictable. Or make some
> other tradeoff like saying we're going to set 10 and you can't assume
> auto-conversion in FlexJS. Either way, we should clean up where we are
> using parseInt(, 10) in the FlexJS framework code.
>
> If you have time, it might be worth doing some tests in Flash and browser
> to see if you can set 10 in Flash and it will still sniff out hex, and
> whether using 0 works the same in both AS and JS. We have set a browser
> baseline of IE9 so we don't have to worry about really old browsers.
>
> Thanks,
> -Alex
>
>
Re: Typedef issue causing flex-asjs to fail to compile
Posted by Alex Harui <ah...@adobe.com>.
On 3/14/17, 10:42 PM, "Greg Dove" <gr...@gmail.com> wrote:
>On Wed, Mar 15, 2017 at 6:33 PM, Alex Harui <ah...@adobe.com> wrote:
>
>> Here's a better link to the old thread. Looks like I did mess around
>>with
>> parseInt earlier.
>>
>> https://s.apache.org/rHq2
>>
>>
>> The goal is to match what AS does and get the performance benefit you
>> measured and maybe prevent browsers from doing something strange.
>>
>> Hopefully it won't require sniffing the first parameter ourselves.
>>
>> Thanks,
>> -Alex
>>
>>
>>
>Thanks for that link, the original was not working for me.
>In theory it should simply take the string and deduce the radix internally
>depending on whether the string has '0x' at the start or not. That's why
>specifying the radix should always be faster.
>But we cannot assume it is 10 for all strings in a compiler generated
>version with the (apparently) mandatory second param for GCL.
>
>So it is a bit of challenge if we want to keep the AS behaviour, and if
>the
>GCL lib does not like having the second arg missing.
>Basically for perfomance you need to specify the second param in the
>native
>version and that obviously will always be the better option.
>I think the compiler generated version will need to do some sniffing
>unfortunately, (if we are to keep behaviour) and will make that
>performance
>difference even more true, but will give it a bit more thought.
>.
I don't think there is any code at runtime that requires the second
argument. We patch the files in the JS.swc so the second argument is
optional. AFAIK, in the actual browser, the second parameter is optional.
IMO, what you see in the original externs is Google's policy for their
developers, not a runtime API spec. But you can see in the old thread
that MDN says you really "should" set a second parameter.
Somehow, that parseInt patch didn't work for Justin and you. So that's
the first problem. Your flex-typedefs/js build should generate a
parseInt.as with an optional second argument because the es3.js is patched
to have an optional second argument.
Then we can discuss whether the compiler should set 10 or 0 or trust the
modern browsers to not think Octal or be unpredictable. Or make some
other tradeoff like saying we're going to set 10 and you can't assume
auto-conversion in FlexJS. Either way, we should clean up where we are
using parseInt(, 10) in the FlexJS framework code.
If you have time, it might be worth doing some tests in Flash and browser
to see if you can set 10 in Flash and it will still sniff out hex, and
whether using 0 works the same in both AS and JS. We have set a browser
baseline of IE9 so we don't have to worry about really old browsers.
Thanks,
-Alex
Re: Typedef issue causing flex-asjs to fail to compile
Posted by Greg Dove <gr...@gmail.com>.
On Wed, Mar 15, 2017 at 6:33 PM, Alex Harui <ah...@adobe.com> wrote:
> Here's a better link to the old thread. Looks like I did mess around with
> parseInt earlier.
>
> https://s.apache.org/rHq2
>
>
> The goal is to match what AS does and get the performance benefit you
> measured and maybe prevent browsers from doing something strange.
>
> Hopefully it won't require sniffing the first parameter ourselves.
>
> Thanks,
> -Alex
>
>
>
Thanks for that link, the original was not working for me.
In theory it should simply take the string and deduce the radix internally
depending on whether the string has '0x' at the start or not. That's why
specifying the radix should always be faster.
But we cannot assume it is 10 for all strings in a compiler generated
version with the (apparently) mandatory second param for GCL.
So it is a bit of challenge if we want to keep the AS behaviour, and if the
GCL lib does not like having the second arg missing.
Basically for perfomance you need to specify the second param in the native
version and that obviously will always be the better option.
I think the compiler generated version will need to do some sniffing
unfortunately, (if we are to keep behaviour) and will make that performance
difference even more true, but will give it a bit more thought.
.
Re: Typedef issue causing flex-asjs to fail to compile
Posted by Alex Harui <ah...@adobe.com>.
Here's a better link to the old thread. Looks like I did mess around with
parseInt earlier.
https://s.apache.org/rHq2
The goal is to match what AS does and get the performance benefit you
measured and maybe prevent browsers from doing something strange.
Hopefully it won't require sniffing the first parameter ourselves.
Thanks,
-Alex
On 3/14/17, 10:20 PM, "Greg Dove" <gr...@gmail.com> wrote:
>>
>> I'm not sure what you are referring to. I don't think the compiler
>> currently supplies the second argument. I didn't think it was needed,
>>but
>> your tests indicate otherwise, so if that's what you are referring to,
>> please file a JIRA so we don't forget to do it.
>>
>>
>>
>It seems it does - it did in that small test I tried and looks like the
>compiler-jx tests are set up to expect this. But I did not look at the
>code
>yet.
>I will add a JIRA
Re: Typedef issue causing flex-asjs to fail to compile
Posted by Greg Dove <gr...@gmail.com>.
>
> I'm not sure what you are referring to. I don't think the compiler
> currently supplies the second argument. I didn't think it was needed, but
> your tests indicate otherwise, so if that's what you are referring to,
> please file a JIRA so we don't forget to do it.
>
>
>
It seems it does - it did in that small test I tried and looks like the
compiler-jx tests are set up to expect this. But I did not look at the code
yet.
I will add a JIRA
Re: Typedef issue causing flex-asjs to fail to compile
Posted by Alex Harui <ah...@adobe.com>.
On 3/14/17, 10:02 PM, "Greg Dove" <gr...@gmail.com> wrote:
>Actually I just checked and it looks like there is a bug in the compiler
>for this
I'm not sure what you are referring to. I don't think the compiler
currently supplies the second argument. I didn't think it was needed, but
your tests indicate otherwise, so if that's what you are referring to,
please file a JIRA so we don't forget to do it.
Thanks,
-Alex
Re: Typedef issue causing flex-asjs to fail to compile
Posted by Alex Harui <ah...@adobe.com>.
On 3/14/17, 10:05 PM, "Greg Dove" <gr...@gmail.com> wrote:
> var test :String = "0x99"
> trace(parseInt(test));
>
>gives :
> var /** @type {string} */ test = "0x99";
> org.apache.flex.utils.Language.trace(parseInt(test, 10));
>
>which is wrong
>
>I can take a look at it this weekend if no-one else gets to it
That would be great. What should the output be?
Also, see this past discussion:
https://lists.apache.org/thread.html/2a36d2d80f49998c5be3df81b4a12408a3d026
01753dedda9042b9ad@1458069451@%3Cdev.flex.apache.org%3E
Thanks,
-Alex
Re: Typedef issue causing flex-asjs to fail to compile
Posted by Greg Dove <gr...@gmail.com>.
var test :String = "0x99"
trace(parseInt(test));
gives :
var /** @type {string} */ test = "0x99";
org.apache.flex.utils.Language.trace(parseInt(test, 10));
which is wrong
I can take a look at it this weekend if no-one else gets to it
On Wed, Mar 15, 2017 at 6:02 PM, Greg Dove <gr...@gmail.com> wrote:
> Actually I just checked and it looks like there is a bug in the compiler
> for this
>
>
>
> Greg Dove
> Dove Software Development Ltd
> http://greg-dove.com
>
> On Wed, Mar 15, 2017 at 5:54 PM, Greg Dove <gr...@gmail.com> wrote:
>
>> Alex, I agree, it seems whatever prompted this was elsewhere, but the
>> outcome is IMO (a small amount of) better framework code in CSSUtils.
>> I would take this as a small win - nothing is broken, and a utility
>> method is theoretically slightly faster.
>>
>>
>> On Wed, Mar 15, 2017 at 5:19 PM, Alex Harui <ah...@adobe.com> wrote:
>>
>>> Thanks for running the test. Maybe I'm not understanding the issue, but
>>> here's my summary.
>>>
>>> Justin was getting a compile error where code that was known to work
>>> wouldn't compile because there was only one argument passed to parseInt
>>> in
>>> ActionScript source.
>>>
>>> ActionScript defines parseInt as having one required parameter and one
>>> default parameter so it should have compiled.
>>>
>>> Thus, the compile error was likely due to the bad typedefs build Justin
>>> referred to earlier in a separate thread.
>>>
>>> It would not be my recommendation to have us add default parameters to
>>> all
>>> of the places we could for "code clarity" or performance. Folks who write
>>> code in ActionScript should know or can find from the documentation that,
>>> for example, the second parameter to parseInt is optional and thus would
>>> wonder why someone bothered to add it. If the second parameter isn't
>>> there, the assumption should be that the default parameter is used.
>>>
>>> Now, if there is a performance advantage to having the output JS always
>>> set 10 if the second parameter is not specified to parseInt, then that
>>> sounds like a good idea. Please file a JIRA so we don't forget.
>>>
>>> But, IMO, we are writing ActionScript and we should not make a practice
>>> of
>>> supplying default parameters. Please figure out why your typedefs aren't
>>> building and remove the optional parameter for parseInt from CSSUtils.as.
>>>
>>> Thanks,
>>> -Alex
>>>
>>>
>>> On 3/14/17, 8:24 PM, "Greg Dove" <gr...@gmail.com> wrote:
>>>
>>> >I think code clarity is one thing, but performance is another - that
>>> >should
>>> >be faster, so I ran a quick check.
>>> >
>>> >I know it can vary across browsers, but
>>> >
>>> >var timeOne = function(){var d=new Date();var b=0; for (var
>>> >i=0;i<10000000;i++) {b= parseInt(""+(127/255)*1000, 10) / 1000;}
>>> >console.log(new Date().getTime()-d.getTime());}
>>> >timeOne()
>>> >approx 715 ms in my chrome over multiple runs
>>> >
>>> >var timeTwo = function(){var d=new Date();var b=0; for (var
>>> >i=0;i<10000000;i++) {b= parseInt(""+(127/255)*1000) / 1000;}
>>> >console.log(new Date().getTime()-d.getTime());}
>>> >timeTwo ()
>>> >approx 870 ms in my chrome over multiple runs
>>> >
>>> >so (within the limits of this *very* basic test) I say keep it, for
>>> >clarity
>>> >and speed (about 20% faster)
>>> >
>>> >On Wed, Mar 15, 2017 at 3:26 PM, Justin Mclean <
>>> justin@classsoftware.com>
>>> >wrote:
>>> >
>>> >> Hi,
>>> >>
>>> >> > Please revert CSSUtils and investigate why parseInt is requiring the
>>> >> second argument.
>>> >>
>>> >> Even if it is a typedef bug IMO passing the base there makes the code
>>> >> intent clearer as the code is dealing with both base 16 and base 10
>>> >>numbers.
>>> >>
>>> >> This is the code in question:
>>> >> public static function attributeFromColor(value:uint):String
>>> >> {
>>> >> var hexVal:String = value.toString(16);
>>> >> if(value > 16777215)
>>> >> {
>>> >> //rgba -- return rgba notation
>>> >> var rgba:Array = hexVal.match(/.{2}/g);
>>> >> for(var i:int = 0; i < 4; i++)
>>> >> {
>>> >> rgba[i] = parseInt(rgba[i], 16);
>>> >> }
>>> >> rgba[3] = parseInt(""+(rgba[3]/255)*1000, 10) / 1000;
>>> >> return "rgba(" + rgba.join(",") + ")";
>>> >> }
>>> >> return "#" + StringPadder.pad(hexVal,"0",6);
>>> >> }
>>> >>
>>> >> I added the “,10” to the second parseInt.
>>> >>
>>> >> What do others think? Should it stay or should it go?
>>> >>
>>> >> Thanks,
>>> >> Justin
>>>
>>>
>>
>
Re: Typedef issue causing flex-asjs to fail to compile
Posted by Greg Dove <gr...@gmail.com>.
Actually I just checked and it looks like there is a bug in the compiler
for this
Greg Dove
Dove Software Development Ltd
http://greg-dove.com
On Wed, Mar 15, 2017 at 5:54 PM, Greg Dove <gr...@gmail.com> wrote:
> Alex, I agree, it seems whatever prompted this was elsewhere, but the
> outcome is IMO (a small amount of) better framework code in CSSUtils.
> I would take this as a small win - nothing is broken, and a utility method
> is theoretically slightly faster.
>
>
> On Wed, Mar 15, 2017 at 5:19 PM, Alex Harui <ah...@adobe.com> wrote:
>
>> Thanks for running the test. Maybe I'm not understanding the issue, but
>> here's my summary.
>>
>> Justin was getting a compile error where code that was known to work
>> wouldn't compile because there was only one argument passed to parseInt in
>> ActionScript source.
>>
>> ActionScript defines parseInt as having one required parameter and one
>> default parameter so it should have compiled.
>>
>> Thus, the compile error was likely due to the bad typedefs build Justin
>> referred to earlier in a separate thread.
>>
>> It would not be my recommendation to have us add default parameters to all
>> of the places we could for "code clarity" or performance. Folks who write
>> code in ActionScript should know or can find from the documentation that,
>> for example, the second parameter to parseInt is optional and thus would
>> wonder why someone bothered to add it. If the second parameter isn't
>> there, the assumption should be that the default parameter is used.
>>
>> Now, if there is a performance advantage to having the output JS always
>> set 10 if the second parameter is not specified to parseInt, then that
>> sounds like a good idea. Please file a JIRA so we don't forget.
>>
>> But, IMO, we are writing ActionScript and we should not make a practice of
>> supplying default parameters. Please figure out why your typedefs aren't
>> building and remove the optional parameter for parseInt from CSSUtils.as.
>>
>> Thanks,
>> -Alex
>>
>>
>> On 3/14/17, 8:24 PM, "Greg Dove" <gr...@gmail.com> wrote:
>>
>> >I think code clarity is one thing, but performance is another - that
>> >should
>> >be faster, so I ran a quick check.
>> >
>> >I know it can vary across browsers, but
>> >
>> >var timeOne = function(){var d=new Date();var b=0; for (var
>> >i=0;i<10000000;i++) {b= parseInt(""+(127/255)*1000, 10) / 1000;}
>> >console.log(new Date().getTime()-d.getTime());}
>> >timeOne()
>> >approx 715 ms in my chrome over multiple runs
>> >
>> >var timeTwo = function(){var d=new Date();var b=0; for (var
>> >i=0;i<10000000;i++) {b= parseInt(""+(127/255)*1000) / 1000;}
>> >console.log(new Date().getTime()-d.getTime());}
>> >timeTwo ()
>> >approx 870 ms in my chrome over multiple runs
>> >
>> >so (within the limits of this *very* basic test) I say keep it, for
>> >clarity
>> >and speed (about 20% faster)
>> >
>> >On Wed, Mar 15, 2017 at 3:26 PM, Justin Mclean <justin@classsoftware.com
>> >
>> >wrote:
>> >
>> >> Hi,
>> >>
>> >> > Please revert CSSUtils and investigate why parseInt is requiring the
>> >> second argument.
>> >>
>> >> Even if it is a typedef bug IMO passing the base there makes the code
>> >> intent clearer as the code is dealing with both base 16 and base 10
>> >>numbers.
>> >>
>> >> This is the code in question:
>> >> public static function attributeFromColor(value:uint):String
>> >> {
>> >> var hexVal:String = value.toString(16);
>> >> if(value > 16777215)
>> >> {
>> >> //rgba -- return rgba notation
>> >> var rgba:Array = hexVal.match(/.{2}/g);
>> >> for(var i:int = 0; i < 4; i++)
>> >> {
>> >> rgba[i] = parseInt(rgba[i], 16);
>> >> }
>> >> rgba[3] = parseInt(""+(rgba[3]/255)*1000, 10) / 1000;
>> >> return "rgba(" + rgba.join(",") + ")";
>> >> }
>> >> return "#" + StringPadder.pad(hexVal,"0",6);
>> >> }
>> >>
>> >> I added the “,10” to the second parseInt.
>> >>
>> >> What do others think? Should it stay or should it go?
>> >>
>> >> Thanks,
>> >> Justin
>>
>>
>
Re: Typedef issue causing flex-asjs to fail to compile
Posted by Greg Dove <gr...@gmail.com>.
If the compiler outputs 10 for everything then it is different to
actionscript which checks for "0x" and decides on 16 or 10
We can change the behaviour, but it won't be identical to :
"Strings beginning with 0x are interpreted as hexadecimal numbers"
Maybe it is better to not support '0x' strings and make developers check
and specify the 16 radix on the substrings....but it is not consistent with
orignal version
On Wed, Mar 15, 2017 at 6:07 PM, Alex Harui <ah...@adobe.com> wrote:
>
>
> On 3/14/17, 9:54 PM, "Greg Dove" <gr...@gmail.com> wrote:
>
> >Alex, I agree, it seems whatever prompted this was elsewhere, but the
> >outcome is IMO (a small amount of) better framework code in CSSUtils.
> >I would take this as a small win - nothing is broken, and a utility method
> >is theoretically slightly faster.
>
> I am still confused. If the compiler should output JS that specifies 10
> for the second parameter if not specified because that will net
> better/faster code we should definitely do that. But I don't understand
> any reason to actually add the second parameter in the ActionScript source.
>
> Thanks,
> -Alex
>
>
Re: Typedef issue causing flex-asjs to fail to compile
Posted by Alex Harui <ah...@adobe.com>.
On 3/14/17, 9:54 PM, "Greg Dove" <gr...@gmail.com> wrote:
>Alex, I agree, it seems whatever prompted this was elsewhere, but the
>outcome is IMO (a small amount of) better framework code in CSSUtils.
>I would take this as a small win - nothing is broken, and a utility method
>is theoretically slightly faster.
I am still confused. If the compiler should output JS that specifies 10
for the second parameter if not specified because that will net
better/faster code we should definitely do that. But I don't understand
any reason to actually add the second parameter in the ActionScript source.
Thanks,
-Alex
Re: Typedef issue causing flex-asjs to fail to compile
Posted by Greg Dove <gr...@gmail.com>.
Alex, I agree, it seems whatever prompted this was elsewhere, but the
outcome is IMO (a small amount of) better framework code in CSSUtils.
I would take this as a small win - nothing is broken, and a utility method
is theoretically slightly faster.
On Wed, Mar 15, 2017 at 5:19 PM, Alex Harui <ah...@adobe.com> wrote:
> Thanks for running the test. Maybe I'm not understanding the issue, but
> here's my summary.
>
> Justin was getting a compile error where code that was known to work
> wouldn't compile because there was only one argument passed to parseInt in
> ActionScript source.
>
> ActionScript defines parseInt as having one required parameter and one
> default parameter so it should have compiled.
>
> Thus, the compile error was likely due to the bad typedefs build Justin
> referred to earlier in a separate thread.
>
> It would not be my recommendation to have us add default parameters to all
> of the places we could for "code clarity" or performance. Folks who write
> code in ActionScript should know or can find from the documentation that,
> for example, the second parameter to parseInt is optional and thus would
> wonder why someone bothered to add it. If the second parameter isn't
> there, the assumption should be that the default parameter is used.
>
> Now, if there is a performance advantage to having the output JS always
> set 10 if the second parameter is not specified to parseInt, then that
> sounds like a good idea. Please file a JIRA so we don't forget.
>
> But, IMO, we are writing ActionScript and we should not make a practice of
> supplying default parameters. Please figure out why your typedefs aren't
> building and remove the optional parameter for parseInt from CSSUtils.as.
>
> Thanks,
> -Alex
>
>
> On 3/14/17, 8:24 PM, "Greg Dove" <gr...@gmail.com> wrote:
>
> >I think code clarity is one thing, but performance is another - that
> >should
> >be faster, so I ran a quick check.
> >
> >I know it can vary across browsers, but
> >
> >var timeOne = function(){var d=new Date();var b=0; for (var
> >i=0;i<10000000;i++) {b= parseInt(""+(127/255)*1000, 10) / 1000;}
> >console.log(new Date().getTime()-d.getTime());}
> >timeOne()
> >approx 715 ms in my chrome over multiple runs
> >
> >var timeTwo = function(){var d=new Date();var b=0; for (var
> >i=0;i<10000000;i++) {b= parseInt(""+(127/255)*1000) / 1000;}
> >console.log(new Date().getTime()-d.getTime());}
> >timeTwo ()
> >approx 870 ms in my chrome over multiple runs
> >
> >so (within the limits of this *very* basic test) I say keep it, for
> >clarity
> >and speed (about 20% faster)
> >
> >On Wed, Mar 15, 2017 at 3:26 PM, Justin Mclean <ju...@classsoftware.com>
> >wrote:
> >
> >> Hi,
> >>
> >> > Please revert CSSUtils and investigate why parseInt is requiring the
> >> second argument.
> >>
> >> Even if it is a typedef bug IMO passing the base there makes the code
> >> intent clearer as the code is dealing with both base 16 and base 10
> >>numbers.
> >>
> >> This is the code in question:
> >> public static function attributeFromColor(value:uint):String
> >> {
> >> var hexVal:String = value.toString(16);
> >> if(value > 16777215)
> >> {
> >> //rgba -- return rgba notation
> >> var rgba:Array = hexVal.match(/.{2}/g);
> >> for(var i:int = 0; i < 4; i++)
> >> {
> >> rgba[i] = parseInt(rgba[i], 16);
> >> }
> >> rgba[3] = parseInt(""+(rgba[3]/255)*1000, 10) / 1000;
> >> return "rgba(" + rgba.join(",") + ")";
> >> }
> >> return "#" + StringPadder.pad(hexVal,"0",6);
> >> }
> >>
> >> I added the “,10” to the second parseInt.
> >>
> >> What do others think? Should it stay or should it go?
> >>
> >> Thanks,
> >> Justin
>
>
Re: Typedef issue causing flex-asjs to fail to compile
Posted by Alex Harui <ah...@adobe.com>.
On 3/14/17, 9:41 PM, "Justin Mclean" <ju...@classsoftware.com> wrote:
>
>The current SDK code uses parseInt(number, “10”) in 29 different places.
>
>This line which I fixed, was the only instance of parseInt(number).
>
>Can you give a valid reason why I should revert it? So far I’m not seeing
>one.
Again, we should not be specifying default parameters where not needed.
It would be better to change those 29 other instances which are probably
old code from before we fixed parseInt.
If your time is limited, file a JIRA to clean up the other 29.
Thanks,
-Alex
Re: Typedef issue causing flex-asjs to fail to compile
Posted by Justin Mclean <ju...@classsoftware.com>.
Hi,
> It would not be my recommendation to have us add default parameters to all
> of the places we could for "code clarity" or performance. Folks who write
> code in ActionScript should know or can find from the documentation that,
> for example, the second parameter to parseInt is optional and thus would
> wonder why someone bothered to add it. If the second parameter isn't
> there, the assumption should be that the default parameter is used.
The current SDK code uses parseInt(number, “10”) in 29 different places.
This line which I fixed, was the only instance of parseInt(number).
Can you give a valid reason why I should revert it? So far I’m not seeing one.
Thanks,
Justin
Re: Typedef issue causing flex-asjs to fail to compile
Posted by Alex Harui <ah...@adobe.com>.
Thanks for running the test. Maybe I'm not understanding the issue, but
here's my summary.
Justin was getting a compile error where code that was known to work
wouldn't compile because there was only one argument passed to parseInt in
ActionScript source.
ActionScript defines parseInt as having one required parameter and one
default parameter so it should have compiled.
Thus, the compile error was likely due to the bad typedefs build Justin
referred to earlier in a separate thread.
It would not be my recommendation to have us add default parameters to all
of the places we could for "code clarity" or performance. Folks who write
code in ActionScript should know or can find from the documentation that,
for example, the second parameter to parseInt is optional and thus would
wonder why someone bothered to add it. If the second parameter isn't
there, the assumption should be that the default parameter is used.
Now, if there is a performance advantage to having the output JS always
set 10 if the second parameter is not specified to parseInt, then that
sounds like a good idea. Please file a JIRA so we don't forget.
But, IMO, we are writing ActionScript and we should not make a practice of
supplying default parameters. Please figure out why your typedefs aren't
building and remove the optional parameter for parseInt from CSSUtils.as.
Thanks,
-Alex
On 3/14/17, 8:24 PM, "Greg Dove" <gr...@gmail.com> wrote:
>I think code clarity is one thing, but performance is another - that
>should
>be faster, so I ran a quick check.
>
>I know it can vary across browsers, but
>
>var timeOne = function(){var d=new Date();var b=0; for (var
>i=0;i<10000000;i++) {b= parseInt(""+(127/255)*1000, 10) / 1000;}
>console.log(new Date().getTime()-d.getTime());}
>timeOne()
>approx 715 ms in my chrome over multiple runs
>
>var timeTwo = function(){var d=new Date();var b=0; for (var
>i=0;i<10000000;i++) {b= parseInt(""+(127/255)*1000) / 1000;}
>console.log(new Date().getTime()-d.getTime());}
>timeTwo ()
>approx 870 ms in my chrome over multiple runs
>
>so (within the limits of this *very* basic test) I say keep it, for
>clarity
>and speed (about 20% faster)
>
>On Wed, Mar 15, 2017 at 3:26 PM, Justin Mclean <ju...@classsoftware.com>
>wrote:
>
>> Hi,
>>
>> > Please revert CSSUtils and investigate why parseInt is requiring the
>> second argument.
>>
>> Even if it is a typedef bug IMO passing the base there makes the code
>> intent clearer as the code is dealing with both base 16 and base 10
>>numbers.
>>
>> This is the code in question:
>> public static function attributeFromColor(value:uint):String
>> {
>> var hexVal:String = value.toString(16);
>> if(value > 16777215)
>> {
>> //rgba -- return rgba notation
>> var rgba:Array = hexVal.match(/.{2}/g);
>> for(var i:int = 0; i < 4; i++)
>> {
>> rgba[i] = parseInt(rgba[i], 16);
>> }
>> rgba[3] = parseInt(""+(rgba[3]/255)*1000, 10) / 1000;
>> return "rgba(" + rgba.join(",") + ")";
>> }
>> return "#" + StringPadder.pad(hexVal,"0",6);
>> }
>>
>> I added the “,10” to the second parseInt.
>>
>> What do others think? Should it stay or should it go?
>>
>> Thanks,
>> Justin
Re: Typedef issue causing flex-asjs to fail to compile
Posted by Greg Dove <gr...@gmail.com>.
I think code clarity is one thing, but performance is another - that should
be faster, so I ran a quick check.
I know it can vary across browsers, but
var timeOne = function(){var d=new Date();var b=0; for (var
i=0;i<10000000;i++) {b= parseInt(""+(127/255)*1000, 10) / 1000;}
console.log(new Date().getTime()-d.getTime());}
timeOne()
approx 715 ms in my chrome over multiple runs
var timeTwo = function(){var d=new Date();var b=0; for (var
i=0;i<10000000;i++) {b= parseInt(""+(127/255)*1000) / 1000;}
console.log(new Date().getTime()-d.getTime());}
timeTwo ()
approx 870 ms in my chrome over multiple runs
so (within the limits of this *very* basic test) I say keep it, for clarity
and speed (about 20% faster)
On Wed, Mar 15, 2017 at 3:26 PM, Justin Mclean <ju...@classsoftware.com>
wrote:
> Hi,
>
> > Please revert CSSUtils and investigate why parseInt is requiring the
> second argument.
>
> Even if it is a typedef bug IMO passing the base there makes the code
> intent clearer as the code is dealing with both base 16 and base 10 numbers.
>
> This is the code in question:
> public static function attributeFromColor(value:uint):String
> {
> var hexVal:String = value.toString(16);
> if(value > 16777215)
> {
> //rgba -- return rgba notation
> var rgba:Array = hexVal.match(/.{2}/g);
> for(var i:int = 0; i < 4; i++)
> {
> rgba[i] = parseInt(rgba[i], 16);
> }
> rgba[3] = parseInt(""+(rgba[3]/255)*1000, 10) / 1000;
> return "rgba(" + rgba.join(",") + ")";
> }
> return "#" + StringPadder.pad(hexVal,"0",6);
> }
>
> I added the “,10” to the second parseInt.
>
> What do others think? Should it stay or should it go?
>
> Thanks,
> Justin
Re: Typedef issue causing flex-asjs to fail to compile
Posted by Justin Mclean <ju...@classsoftware.com>.
Hi,
> Please revert CSSUtils and investigate why parseInt is requiring the second argument.
Even if it is a typedef bug IMO passing the base there makes the code intent clearer as the code is dealing with both base 16 and base 10 numbers.
This is the code in question:
public static function attributeFromColor(value:uint):String
{
var hexVal:String = value.toString(16);
if(value > 16777215)
{
//rgba -- return rgba notation
var rgba:Array = hexVal.match(/.{2}/g);
for(var i:int = 0; i < 4; i++)
{
rgba[i] = parseInt(rgba[i], 16);
}
rgba[3] = parseInt(""+(rgba[3]/255)*1000, 10) / 1000;
return "rgba(" + rgba.join(",") + ")";
}
return "#" + StringPadder.pad(hexVal,"0",6);
}
I added the “,10” to the second parseInt.
What do others think? Should it stay or should it go?
Thanks,
Justin
Re: Typedef issue causing flex-asjs to fail to compile
Posted by Alex Harui <ah...@adobe.com>.
Hmm. That would be a bug in the typedefs build. The Ant build attempts
to fix up parseInt to make the second parameter optional. AS developers
shouldn't have to add the second argument. Please revert CSSUtils and
investigate why parseInt is requiring the second argument.
It is strange that all of this is working on builds.a.o.
In flex-typedefs, the generated parseint.as should look like:
public function parseInt(num:*, base:Number = 0):Number { return
null; }
Because before the SWC compile, the es3.js should be patched to look like:
function parseInt(num, opt_base) {}
Thanks,
-Alex
On 3/14/17, 4:01 PM, "Justin Mclean" <ju...@classsoftware.com> wrote:
>Hi,
>
>There also looks to be a typedef issue. Current CCSUtils is failing to
>compile:
>
>/Users/justinmclean/flex-asjs/frameworks/projects/Core/src/main/flex/org/a
>pache/flex/utils/CSSUtils.as(52): col: 27 Incorrect number of arguments.
>Expected 2
>
> rgba[3] = parseInt(""+(rgba[3]/255)*1000) / 1000;
> ^
>
>The typedef has this comment which seems a clue:
>/**
> * Parse an integer. Use of {@code parseInt} without {@code base} is
>strictly
> * banned in Google. If you really want to parse octal or hex based
>on the
> * leader, then pass {@code undefined} as the base.
> *
>
>Thanks,
>Justin