[webkit-reviews] review denied: [Bug 10878] Incorrect decompilation for "4..x" : [Attachment 14441] Fix the 4..x toString output by grouping the number if it's in dotted expression

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed May 9 19:26:11 PDT 2007


Darin Adler <darin at apple.com> has denied Kimmo Kinnunen <kimmok at iki.fi>'s
request for review:
Bug 10878: Incorrect decompilation for "4..x"
http://bugs.webkit.org/show_bug.cgi?id=10878

Attachment 14441: Fix the 4..x toString output by grouping the number if it's
in dotted expression
http://bugs.webkit.org/attachment.cgi?id=14441&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
+    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.



More information about the webkit-reviews mailing list