Opened 10 years ago

Closed 10 years ago

#2478 closed enhancement (invalid)

[PATCH] Eliminating "for each" statements with Array.forEach

Reported by: False Vision Owned by:
Priority: Nice to Have Milestone:
Component: Core engine Keywords: patch
Cc: Patch:

Description

The "for each...in" statement is deprecated as the part of ECMA-357 and currently only SpiderMoney supports it.

People like me want to run the game on other JavaScript engines and for each statements won't run on them.

I am porting the game to another platform so it would be great if we simply eliminate for each statements with Array.forEach.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for_each...in

Patch is attached.

Attachments (1)

HelperScripts.patch (17.3 KB ) - added by False Vision 10 years ago.

Download all attachments as: .zip

Change History (7)

by False Vision, 10 years ago

Attachment: HelperScripts.patch added

in reply to:  description comment:1 by historic_bruno, 10 years ago

Replying to falsevision:

People like me want to run the game on other JavaScript engines

I'm just curious, but why? I've always thought it would be interesting if parts of the game could e.g. run in a browser, but I've never seen a practical application of that (other than maybe Philip doing some testing or proof of concepts), and no one has expressed much interest in it.

You should probably know about #2475. Both that and for each were discussed in yesterday's staff meeting, you can read about here.

comment:2 by False Vision, 10 years ago

"for each" statement is only supported on Mozilla Firefox so any effort to run the game on a browser will fail.

But good news is that "for of" will probably replace "for each" in ECMAScript 6. So we would have two "for"s:

for(a in b) // for keys
for(a of b) // for values

"for of" is currently supported by Spider Monkey (experimental) and i could run it on 0.A.D. I believe my patch is so dirty because there are lots of "continue", "break" and "return" statements in body of the loops so implementing them with Array.forEach(function func) is a pain and increases the complexity of the code.

I am porting the game to Windows Phone 8.1 and i am using Jint as scripting engine which doesn't support "for each". Hopefully it is open source and i could modify the source code to add support for both "for each" and "for of" in less than 30 minutes.

Now i do not need the patched to be applied and i think it's better to wait for "for of" to officially put in ECMAScript 6. Or you might want to replace "for each" with "for of" now because none of them is standard (yet) and there would be no difference.

Please skip my patch.

in reply to:  2 comment:3 by historic_bruno, 10 years ago

Keywords: review removed

Replying to falsevision:

I am porting the game to Windows Phone 8.1 and i am using Jint as scripting engine which doesn't support "for each".

That's interesting. Someone else was also porting the game to Windows apps, but they were using C# for everything, which I think was a bad idea. Are you still using C++? Is it a complete rewrite or just adapting the existing engine?

I agree about your patch, but we can keep this ticket open as a reminder about the for each issue.

comment:4 by sanderd17, 10 years ago

Imo, we should go with for..of in most cases, as that allows breaking, and looks abit nicer. Only the important stuff for performance should use forEach().

But I think, from now on, we'll use for..of in new code, and later on, the old code can also be updated.

Though I warn you, we still only develop for spidermonkey, and only test on that platform. So any compatibility problems will be for you.

Last edited 10 years ago by sanderd17 (previous) (diff)

comment:5 by agentx, 10 years ago

Performance-wise a while(){} is the fastest. If case of for(i=0;i<a.length;i++){} a.length should be put in a variable upfront as the lookup is too costly on each iteration.

comment:6 by sanderd17, 10 years ago

Milestone: Alpha 16
Resolution: invalid
Status: newclosed

We will probably slowly migrate to for..of. But there's no reason to make a patch for it yet. We clearly state in the coding styles that SpiderMonkey extensions can be used.

Note: See TracTickets for help on using tickets.