http://codereview.chromium.org/2801018/diff/54003/66007
File src/ia32/stub-cache-ia32.cc (right):
http://codereview.chromium.org/2801018/diff/54003/66007#newcode104
src/ia32/stub-cache-ia32.cc:104: // Helper function used to check that
the dictionary doesn't contain the prop.
On 2010/07/01 10:11:29, Mads Ager wrote:
prop -> property.
Done.
http://codereview.chromium.org/2801018/diff/54003/66007#newcode172
src/ia32/stub-cache-ia32.cc:172: // Having undefined at this place means
the name is not contained.
On 2010/07/01 10:11:29, Mads Ager wrote:
Please expand this comment to state that the undefined check takes
care of both
present and deleted elements because deleted elements use null as the
name.
I woudn't say this is the place which takes care of deleted elements.
Instead I added a comment above describing why this method is correct
and why deleted values don't break it.
http://codereview.chromium.org/2801018/diff/54003/66007#newcode177
src/ia32/stub-cache-ia32.cc:177: if (entity_name.is(properties)) {
On 2010/07/01 10:11:29, Mads Ager wrote:
Could you use extra.is(no_reg) for consistency with the condition for
pushing
the receiver?
Done.
http://codereview.chromium.org/2801018/diff/54003/66007#newcode187
src/ia32/stub-cache-ia32.cc:187: if (entity_name.is(properties)) {
On 2010/07/01 10:11:29, Mads Ager wrote:
Could you use extra.is(no_reg) here as well?
Done.
http://codereview.chromium.org/2801018/diff/54003/66007#newcode828
src/ia32/stub-cache-ia32.cc:828: Register
StubCompiler::CheckPrototypes(JSObject* object,
On 2010/07/01 10:11:29, Mads Ager wrote:
Comments should be added to CheckPrototypes to state what it does.
Folding
CheckMaps into CheckPrototypes makes sense, but we should document
what
CheckPrototypes does.
Please document register use, push_at_depth, and what can be expected
as a
result of calling CheckPrototypes. Starting from the CheckMaps comment
that has
now been removed would make sense.
Added comments to stub-cache.h.
http://codereview.chromium.org/2801018/diff/54003/66007#newcode839
src/ia32/stub-cache-ia32.cc:839: // registers.
On 2010/07/01 10:11:29, Mads Ager wrote:
other registers -> holder and object registers.
You do not check extra and you probably don't have to?
Done.
http://codereview.chromium.org/2801018/diff/54003/66007#newcode930
src/ia32/stub-cache-ia32.cc:930: // Check that the object layout has not
changed (if it's a fast properties
On 2010/07/01 10:11:29, Mads Ager wrote:
Could you go back to the original "Check the holder map." comment
there. I don't
find the comment that you have now useful. The map check implies a lot
of things
and not just for properties of type CONSTANT_FUNCTION. It does not
imply that
the object is the same.
Done.
http://codereview.chromium.org/2801018/diff/54003/66007#newcode940
src/ia32/stub-cache-ia32.cc:940: // Perform security check for access to
the global object and return
On 2010/07/01 10:11:29, Mads Ager wrote:
Remove the 'return the holder register part' since you do not do this
yet.
Done.
http://codereview.chromium.org/2801018/diff/54003/66007#newcode948
src/ia32/stub-cache-ia32.cc:948: current = object;
On 2010/07/01 10:11:29, Mads Ager wrote:
Please reinstate the comment about having to check for global property
cell
equality if we have skipped any global objects.
Done.
I still think it would make sense to split this into a couple of
helper methods
to make this code easier to read. This method could be something like:
CheckPrototypes:
if receiver is slow case:
NegativeDictionaryLookup on receiver
object = receiver->GetPrototype()
CheckMaps from object to holder
CheckGlobalPropertyCells
I would find that much easier to read as the user of this method. Then
you can
dive into the details of the individual parts if you want to.
I extracted methods GenerateCheckPropertyHolder and
GenerateCheckPropertyCells, but I I left the main loop here. I still
dislike the CheckMaps method (one more reason I dislike it is the
push_at_depth argument). Would be cool to move a piece of the loop body
in to a method but I don't see how to do it keeping its optimizations.
I'l think for a while.
http://codereview.chromium.org/2801018/diff/54003/66008
File src/ic-inl.h (right):
http://codereview.chromium.org/2801018/diff/54003/66008#newcode99
src/ic-inl.h:99: // the same prototype (or a prototypes with the same
map) and not having
On 2010/07/01 10:11:29, Mads Ager wrote:
or a prototypes -> or a prototype
Done.
http://codereview.chromium.org/2801018/show
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
