[Webkit-unassigned] [Bug 6252] JavaScript 1.6 Array.lastIndexOf

bugzilla-daemon at opendarwin.org bugzilla-daemon at opendarwin.org
Fri Aug 18 08:28:53 PDT 2006


http://bugzilla.opendarwin.org/show_bug.cgi?id=6252


darin at apple.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #10095|review?                     |review-
               Flag|                            |




------- Comment #14 from darin at apple.com  2006-08-18 08:28 PDT -------
(From update of attachment 10095)
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.


-- 
Configure bugmail: http://bugzilla.opendarwin.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.



More information about the webkit-unassigned mailing list