[webkit-reviews] review denied: [Bug 6252] JavaScript 1.6 Array.lastIndexOf : [Attachment 10095] lastIndexOf javascript patch

[bugzilla-request-daemon at opendarwin.org]

Darin Adler <darin at apple.com> has denied Darin Adler <darin at apple.com>'s
request for review:
Bug 6252: JavaScript 1.6 Array.lastIndexOf
http://bugzilla.opendarwin.org/show_bug.cgi?id=6252

Attachment 10095: lastIndexOf javascript patch
http://bugzilla.opendarwin.org/attachment.cgi?id=10095&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
Generally looks good!

I see a number of minor mistakes:

+	   index = static_cast<unsigned> (d);

It makes no sense to cast d to unsigned here. The types involved are int and
double. This should be changed to a cast to int. Also, we don't use a space
after the ">" and before the "(".

+      if ((d + length) < 0)
+	   return jsNumber(-1);
+      if (d < 0)
+	   d += length;      

I think it's a shame that this adds length to d twice. How about this instead:

    if (d < 0) {
	d += length;	  
	if (d < 0)
	    return jsNumber(-1);
    }

Or it could be structured more like the IndexOf version above.

+      for (; index >= 0; --index) {
+	   JSValue* e = getProperty(exec, thisObj, index);
+	   if (!e)
+	       e = jsUndefined();
+	   if (strictEqual(exec, searchElement, e))
+	   return jsNumber(index);
+	   }

Indentation above is incorrect. The return statement needs to be indented and
the ending brace needs to be outdented.

There are other indentation problems: The top two comment lines are not
indented the same amount, the ending brace for the entire case is at the wrong
level, the return statement doesn't line up with the rest of the code, and
there's a stray tab character in the patch (so we'll get an error due to the
pre-commit test that checks for tabs).

I'd also like to see a new test that tests all the different edge cases. It's
nice that this fixes some JavaScript tests, but ideally you'd have a new test
like the one we added when we added indexOf (see
LayoutTests/fast/js/array-indexof.html for that one).
None of these are major, but I'm going to mark this review- so that we can fix
some or all of these issues. The biggest one for me would be having a test.