[webkit-reviews] review denied: [Bug 42857] Inefficient Handling of Shift and Unshift : [Attachment 62360] Patch file

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Jul 26 16:17:24 PDT 2010


Gavin Barraclough <barraclough at apple.com> has denied Michael Saboff
<msaboff at apple.com>'s request for review:
Bug 42857: Inefficient Handling of Shift and Unshift
https://bugs.webkit.org/show_bug.cgi?id=42857

Attachment 62360: Patch file
https://bugs.webkit.org/attachment.cgi?id=62360&action=review

------- Additional Comments from Gavin Barraclough <barraclough at apple.com>
Please revert unnecessary change to
JavaScriptCore/Configurations/JavaScriptCore.xcconfig.

There are a couple of these:
    BaseIndex(regT2, regT1, ScalePtr, 0)
You should just remove the last ",0", this is not necessary.

In JSArray.h, typo: "qucik"
    "return (ArrayStorage *)((char*)" - these should be c++ style casts.

in ArrayPrototype.cpp:
    We have a coding style rule that we don't use abbreviations, unless it is
canonically understood.  I don't think the 'nr' in nrArgs matches the style in
the codebase well, and I think it falls foul of this rule.  We do commonly use
the abbreviations 'args' and 'num' for number.	I'd suggest numArgs,
numberOfArgs, argsCount, etc could all be acceptable, but I think 'nr' should
change.  I'd also prefer 'index' to 'k' as the loop counter, but single
character loop index names clearly has precedent.

Otherwise, all looks great – normally I'd r+, but I'll r- despite the small
number of changes since you'll be needing to put up a new patch for the
commit-queue anyway.


More information about the webkit-reviews mailing list