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

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has granted Gavin Barraclough
<barraclough at apple.com>'s request for review:
Bug 20843: Accelerated property accesses.
https://bugs.webkit.org/show_bug.cgi?id=20843

Attachment 23416: The patch
https://bugs.webkit.org/attachment.cgi?id=23416&action=edit

------- 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
land.
+#define INVALID_POINTER_VALUE 0xF0F0F0F0
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.


+	 #define PROPERTY_ACCESS_OFFSET_PUT_STRUCTUREID 19
+	 #define PROPERTY_ACCESS_OFFSET_PUT_OFFSET 34
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
face.
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.