Bug 10878 – Incorrect decompilation for "4..x"

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Jesse Ruderman]

javascript:alert(function(){alert(4..x)})

decompiles with "4.x", which is a syntax error.

Here are some decompilations that I think would be correct; please choose one ;)

(4).x  (this is what Firefox trunk does)
4..x
4 .x

[Kimmo Kinnunen]

Created attachment 14441 [details]
Fix the 4..x toString output by grouping the number if it's in dotted expression

[Eric Seidel (no email)]

I still don't 100% understand this bug (since I don't really understand what 4..x is supposed to mean).  But in your fix, groupIfNumber should be m_groupIfNumber, I'm pretty sure the goal is to eventually move JSC to more modern conventions.

[Mark Rowe (bdash)]

Comment on attachment 14441 [details]
Fix the 4..x toString output by grouping the number if it's in dotted expression

In this middle of your patch you have the following, which looks unrelated and incorrect:

-  for (int i = 0; i < elision; i++)
+  for (int i = 0; i = elision; i++)

[Darin Adler]

Comment on attachment 14441 [details]
Fix the 4..x toString output by grouping the number if it's in dotted expression

+    SourceStream(): groupIfNumber(false) {};

No semicolon needed here. We normally put a space before the colon that introduces the initializer list.

+  groupIfNumber = false;
   str += s.ustring();
+  groupIfNumber = false;

Why do you need to set the variable twice here?

+  for (int i = 0; i = elision; i++)

This is wrong. The loop condition is an assignment statement. You'll end up with an infinite loop. Why didn't a test case catch this? Also, why the change at all? This seems to be an unnecessary change.

+  if (groupIfNumber) {
+    UChar ch('(');
+    str += UString(&ch, 1);
+  }

I think this would read nicer as:

    if (groupIfNumber)
        str.append("(");

No need to use a UChar local variable. You can use += if you prefer it over append(), but I suggest "" instead of the UChar.

[Kimmo Kinnunen]

Created attachment 14489 [details]
Second try, address the comments

[Darin Adler]

Comment on attachment 14489 [details]
Second try, address the comments

r=me

I noticed that the test case doesn't cover all the code. For example, there are many lines of code in nodes2string.cpp that you could delete without breaking any test case. I suggest making an even better test case that covers every single case at some point, but no need to do this before landing this.

Typo: "formating".

[Mark Rowe (bdash)]

Landed in r21406, tests in r21407 due to operator error.

Kimmo, can you be sure to include a ChangeLog entry for your layout test changes in future?

[Kimmo Kinnunen]

Created attachment 14543 [details]
Improved testcases 

The patch contains improved testcases. It tries to cover all the parse tree nodes which were modified in the previous patch.
Sorry about omitting LayoutTests/ChangeLog. A mistake.

[Darin Adler]

Comment on attachment 14543 [details]
Improved testcases 

Please also don't use tabs in your patches. We don't use tabs and whoever goes to land your changes will have to replace them with spaces so Subversion will accept the patch.

This is very thorough. Great!

I think it would be even better if the test case output reflected the test cases instead of just f1 through f6. To do that you need to structure the test so the strings you pass to shouldBe contain the interesting part of the test.