Sergey, I get consistent '500 Server Error' messages when attempting to access this codereview (but I can access other code reviews just fine). Could you attempt another upload and if that does not work create a new one?
Thanks, -- Mads On Thu, Jul 1, 2010 at 9:21 PM, <[email protected]> wrote: > > 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
