Handlify upper layers of LoadIC.

Handlify upper layers of LoadIC.

BUG=
TEST=

Committed: http://code.google.com/p/v8/source/detail?r=9680

[ulan, 2011-10-18 08:06:23]

PTAL.

[Kevin Millikin (Chromium), 2011-10-18 08:46:47]

LGTM with comments below.  Remember that we need GC support for pointers in
LookupResults before landing this.

http://codereview.chromium.org/8337008/diff/1/src/ic.cc
File src/ic.cc (left):

http://codereview.chromium.org/8337008/diff/1/src/ic.cc#oldcode374
src/ic.cc:374: AssertNoAllocation no_gc;  // pointers must stay valid
Can we restore this assert scope in the raw pointer version of this function?

http://codereview.chromium.org/8337008/diff/1/src/ic.cc
File src/ic.cc (right):

http://codereview.chromium.org/8337008/diff/1/src/ic.cc#newcode938
src/ic.cc:938: Handle<Code> stub;
Indentation of the body is wrong (too far to the right).

http://codereview.chromium.org/8337008/diff/1/src/ic.cc#newcode1081
src/ic.cc:1081: isolate()->stub_cache()->Set(*name, *map, *code);
I guess it doesn't make much sense to create a handle to the map only to
dereference it immediately.

Lets do:

isolate()->stub_cache()->Set(*name, receiver->map(), *code);

for now.

http://codereview.chromium.org/8337008/diff/1/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/8337008/diff/1/src/objects.h#newcode916
src/objects.h:916: static Handle<Object> GetProperty(Isolate* isolate,
I like the static functions for namespacing.  We should, at the same time,
remove the corresponding toplevel functions in handles.h.  In this case, we
should be able to get rid of

Handle<Object> GetProperty(Handle<JSReceiver>, Handle<String>, LookupResult*);

http://codereview.chromium.org/8337008/diff/1/src/stub-cache.cc
File src/stub-cache.cc (right):

http://codereview.chromium.org/8337008/diff/1/src/stub-cache.cc#newcode142
src/stub-cache.cc:142: if (probe->IsCode()) return Handle<Code>::cast(probe);
I like a blank line right after this one, to highlight the early return.  Also,
everywhere below.

http://codereview.chromium.org/8337008/diff/1/src/stub-cache.cc#newcode147
src/stub-cache.cc:147: GDBJIT(AddCode(GDBJITInterface::LOAD_IC, *cache_name,
*code));
I think that this code is ignored unless we set up the preprocessor to compile
GDBJIT code.  We should verify it compiles so we don't have a type error here.

http://codereview.chromium.org/8337008/diff/1/src/stub-cache.h
File src/stub-cache.h (right):

http://codereview.chromium.org/8337008/diff/1/src/stub-cache.h#newcode693
src/stub-cache.h:693: explicit StoreStubCompiler(Isolate* isolate,
StrictModeFlag strict_mode)
No need to explicit for two-argument constructors.

http://codereview.chromium.org/8337008/diff/1/src/stub-cache.h#newcode720
src/stub-cache.h:720: explicit KeyedStoreStubCompiler(Isolate* isolate,
StrictModeFlag strict_mode)
Same, you can drop explicit.

[ulan, 2011-10-18 09:31:00]

Please take another look. All comments except gdbjit are addressed.

http://codereview.chromium.org/8337008/diff/1/src/ic.cc
File src/ic.cc (left):

http://codereview.chromium.org/8337008/diff/1/src/ic.cc#oldcode374
src/ic.cc:374: AssertNoAllocation no_gc;  // pointers must stay valid
On 2011/10/18 08:46:47, Kevin Millikin wrote:
> Can we restore this assert scope in the raw pointer version of this function?

Done.

http://codereview.chromium.org/8337008/diff/1/src/ic.cc
File src/ic.cc (right):

http://codereview.chromium.org/8337008/diff/1/src/ic.cc#newcode938
src/ic.cc:938: Handle<Code> stub;
On 2011/10/18 08:46:47, Kevin Millikin wrote:
> Indentation of the body is wrong (too far to the right).

Done.

http://codereview.chromium.org/8337008/diff/1/src/ic.cc#newcode1081
src/ic.cc:1081: isolate()->stub_cache()->Set(*name, *map, *code);
On 2011/10/18 08:46:47, Kevin Millikin wrote:
> I guess it doesn't make much sense to create a handle to the map only to
> dereference it immediately.
> 
> Lets do:
> 
> isolate()->stub_cache()->Set(*name, receiver->map(), *code);
> 
> for now.

Done.

http://codereview.chromium.org/8337008/diff/1/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/8337008/diff/1/src/objects.h#newcode916
src/objects.h:916: static Handle<Object> GetProperty(Isolate* isolate,
On 2011/10/18 08:46:47, Kevin Millikin wrote:
> I like the static functions for namespacing.  We should, at the same time,
> remove the corresponding toplevel functions in handles.h.  In this case, we
> should be able to get rid of
> 
> Handle<Object> GetProperty(Handle<JSReceiver>, Handle<String>, LookupResult*);

Done.

http://codereview.chromium.org/8337008/diff/1/src/stub-cache.cc
File src/stub-cache.cc (right):

http://codereview.chromium.org/8337008/diff/1/src/stub-cache.cc#newcode142
src/stub-cache.cc:142: if (probe->IsCode()) return Handle<Code>::cast(probe);
On 2011/10/18 08:46:47, Kevin Millikin wrote:
> I like a blank line right after this one, to highlight the early return. 
Also,
> everywhere below.

Done.

http://codereview.chromium.org/8337008/diff/1/src/stub-cache.h
File src/stub-cache.h (right):

http://codereview.chromium.org/8337008/diff/1/src/stub-cache.h#newcode693
src/stub-cache.h:693: explicit StoreStubCompiler(Isolate* isolate,
StrictModeFlag strict_mode)
On 2011/10/18 08:46:47, Kevin Millikin wrote:
> No need to explicit for two-argument constructors.

Done.

http://codereview.chromium.org/8337008/diff/1/src/stub-cache.h#newcode720
src/stub-cache.h:720: explicit KeyedStoreStubCompiler(Isolate* isolate,
StrictModeFlag strict_mode)
On 2011/10/18 08:46:47, Kevin Millikin wrote:
> Same, you can drop explicit.

Done.

[Kevin Millikin (Chromium), 2011-10-18 09:36:34]

lgtm