[webkit-reviews] review granted: [Bug 20843] Accelerated property accesses. : [Attachment 23416] The patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Sep 14 17:08:04 PDT 2008

Sam Weinig <sam at webkit.org> has granted Gavin Barraclough
<barraclough at apple.com>'s request for review:
Bug 20843: Accelerated property accesses.

Attachment 23416: The patch

------- Additional Comments from Sam Weinig <sam at webkit.org>
The changelog should have perf numbers as is our norm.

+// This will not sign extend from 8bit, and should be somewhere up in kernel
This should be below the includes.  Is this value correct for the mac (bdash
thinks perhaps not).  Can it be a const int.

structureStubCompilationInfo should be m_ structureStubCompilationInfo

structIDInstrIdx is an odd variable name.  I think it can be more verbose.

Can these be constants instead of #defines.  Same goes for a few others just
like it.

There are a bunch of c-style casts.  Please convert to use c++-style to make
cpst happy (and me!).

#if USE(CTI_REPATCH_PIC) && 0 // This is not currently a performance win.  Sad
We usually don't commit things like this.

Please replace all TODOs with FIXMEs.

Overall, nothing popped out as being wrong, but it is a lot of code.  I think
an extra pair of eyes would be good.  And for the future generations, perhaps a
more detailed changelog, explaining exactly what you mean by re-patching.

r=me.  But I think it might be wise to let someone else r+ as well.

More information about the webkit-reviews mailing list