Bug 10878 - Incorrect decompilation for "4..x"
Summary: Incorrect decompilation for "4..x"
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 420+
Hardware: Macintosh OS X 10.4
: P2 Minor
Assignee: Nobody
URL:
Keywords: HasReduction
Depends on:
Blocks: 13638
  Show dependency treegraph
 
Reported: 2006-09-15 23:18 PDT by Jesse Ruderman
Modified: 2007-05-14 06:47 PDT (History)
1 user (show)

See Also:


Attachments
Fix the 4..x toString output by grouping the number if it's in dotted expression (9.66 KB, patch)
2007-05-09 13:40 PDT, Kimmo Kinnunen
darin: review-
Details | Formatted Diff | Diff
Second try, address the comments (8.76 KB, patch)
2007-05-11 07:02 PDT, Kimmo Kinnunen
darin: review+
Details | Formatted Diff | Diff
Improved testcases (6.87 KB, patch)
2007-05-14 06:35 PDT, Kimmo Kinnunen
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jesse Ruderman 2006-09-15 23:18:38 PDT
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
Comment 1 Kimmo Kinnunen 2007-05-09 13:40:21 PDT
Created attachment 14441 [details]
Fix the 4..x toString output by grouping the number if it's in dotted expression
Comment 2 Eric Seidel (no email) 2007-05-09 16:42:29 PDT
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.
Comment 3 Mark Rowe (bdash) 2007-05-09 17:28:21 PDT
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++)
Comment 4 Darin Adler 2007-05-09 19:26:11 PDT
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.
Comment 5 Kimmo Kinnunen 2007-05-11 07:02:00 PDT
Created attachment 14489 [details]
Second try, address the comments
Comment 6 Darin Adler 2007-05-11 08:13:10 PDT
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".
Comment 7 Mark Rowe (bdash) 2007-05-11 09:40:11 PDT
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?
Comment 8 Kimmo Kinnunen 2007-05-14 06:35:00 PDT
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.
Comment 9 Darin Adler 2007-05-14 06:47:25 PDT
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.