A few more comments.
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. prop -> property. 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. 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. http://codereview.chromium.org/2801018/diff/54003/66007#newcode177 src/ia32/stub-cache-ia32.cc:177: if (entity_name.is(properties)) { Could you use extra.is(no_reg) for consistency with the condition for pushing the receiver? http://codereview.chromium.org/2801018/diff/54003/66007#newcode187 src/ia32/stub-cache-ia32.cc:187: if (entity_name.is(properties)) { Could you use extra.is(no_reg) here as well? http://codereview.chromium.org/2801018/diff/54003/66007#newcode828 src/ia32/stub-cache-ia32.cc:828: Register StubCompiler::CheckPrototypes(JSObject* object, 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. http://codereview.chromium.org/2801018/diff/54003/66007#newcode839 src/ia32/stub-cache-ia32.cc:839: // registers. other registers -> holder and object registers. You do not check extra and you probably don't have to? 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 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. 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 Remove the 'return the holder register part' since you do not do this yet. http://codereview.chromium.org/2801018/diff/54003/66007#newcode948 src/ia32/stub-cache-ia32.cc:948: current = object; Please reinstate the comment about having to check for global property cell equality if we have skipped any global objects. 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. 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 or a prototypes -> or a prototype http://codereview.chromium.org/2801018/show -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
