Thanks a lot for review, Mads!
http://codereview.chromium.org/118118/diff/1/4 File src/ia32/stub-cache-ia32.cc (right): http://codereview.chromium.org/118118/diff/1/4#newcode363 Line 363: // TODO(367): maybe don't push lookup_hint for LOOKUP_IN_HOLDER and/or On 2009/06/02 17:14:04, Mads Ager wrote: > We normally capitalize the start of these TODO(xxx): messages: 'maybe' -> > 'Maybe'. Done. http://codereview.chromium.org/118118/diff/1/4#newcode1001 Line 1001: // TODO(368): compile in the whole chain: all the interceptors in On 2009/06/02 17:14:04, Mads Ager wrote: > Capitalize for consistency with other TODOs: 'compile' -> 'Compile'. Done. http://codereview.chromium.org/118118/diff/1/7 File src/objects-inl.h (right): http://codereview.chromium.org/118118/diff/1/7#newcode2558 Line 2558: // TODO(antonm): do we want to do any shortcuts for global object? On 2009/06/02 17:14:04, Mads Ager wrote: > I'm not sure I understand this TODO. Could you elaborate (and potentially file > a bug report and use the number)? Sure. The code below is pretty close to JSObject::LocalLookupRealNamedProperty, but doesn't have the check IsJSGlobalProxy() (cf. with the method), so I wonder if it should be here as well. BTW, what do you think about it? http://codereview.chromium.org/118118/diff/1/3 File src/objects.cc (right): http://codereview.chromium.org/118118/diff/1/3#newcode5621 Line 5621: JSObject* receiver, String* name, PropertyAttributes* attributes) { On 2009/06/02 17:14:04, Mads Ager wrote: > Use one line per argument here (as in GetInterceptorPropertyWithLookupHint)? The problem is that if formatted as method_name(arg1, arg2 it goes out of 80 limit. What would you say about this variant? http://codereview.chromium.org/118118/diff/1/3#newcode5664 Line 5664: RETURN_IF_SCHEDULED_EXCEPTION(); On 2009/06/02 17:14:04, Mads Ager wrote: > I'm not sure this one is needed? I think that GetPropertyWithInterceptorProper > handles scheduled exceptions by returning Failure::Exception. In that case, you > should just propatate that out which is done by the return in the next line. Aha, thanks. Done. http://codereview.chromium.org/118118/diff/1/3#newcode5676 Line 5676: RETURN_IF_SCHEDULED_EXCEPTION(); On 2009/06/02 17:14:04, Mads Ager wrote: > Is this one needed? What here can cause an exception to be scheduled? Sorry, should have swapped with the line below. http://codereview.chromium.org/118118/diff/1/3#newcode5702 Line 5702: JSObject* receiver, String* name, PropertyAttributes* attributes) { On 2009/06/02 17:14:04, Mads Ager wrote: > One line per argument? Done. Same problem as above: otherwise breaks 80 limit http://codereview.chromium.org/118118/diff/1/3#newcode5709 Line 5709: RETURN_IF_SCHEDULED_EXCEPTION(); On 2009/06/02 17:14:04, Mads Ager wrote: > Is this one needed or is it handled by GetPropertyWithInterceptorProper already? Done. http://codereview.chromium.org/118118/diff/1/6 File src/objects.h (right): http://codereview.chromium.org/118118/diff/1/6#newcode1354 Line 1354: Smi* payload, On 2009/06/02 17:14:04, Mads Ager wrote: > 'payload' -> 'hint'? Done. http://codereview.chromium.org/118118/diff/1/6#newcode1357 Line 1357: enum { LOOKUP_IN_HOLDER = -1, LOOKUP_IN_PROTOTYPE = -2 }; On 2009/06/02 17:14:04, Mads Ager wrote: > For consistency with the rest of the code I would add these as constants to the > list of constants later in the class definition (kMaxGap and friends). > static const int kLookupInHolderHint = -1; > static const int kLookupInPrototypeHint = -2; Done. http://codereview.chromium.org/118118/diff/1/8 File src/stub-cache.h (right): http://codereview.chromium.org/118118/diff/1/8#newcode363 Line 363: Smi* lookup_payload, On 2009/06/02 17:14:04, Mads Ager wrote: > lookup_payload -> lookup_hint? Done. http://codereview.chromium.org/118118 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
