Handlify upper layers of StoreIC based on 8337008.

Handlify upper layers of StoreIC based on 8337008.

BUG=
TEST=

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

[ulan, 2011-10-18 08:18:27]

PTAL.

[Kevin Millikin (Chromium), 2011-10-18 11:39:40]

LGTM if comments are addressed.

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

http://codereview.chromium.org/8341008/diff/1/src/ic.cc#newcode1423
src/ic.cc:1423: HandleScope scope(isolate());
This handle scope is not really necessary because of the outer one.  Let's
remove it.

http://codereview.chromium.org/8341008/diff/1/src/ic.cc#newcode1424
src/ic.cc:1424: Handle<Object> result = SetElement(receiver, index, value,
strict_mode);
As part of this cleanup, we should probably move this SetElement to a static
member function of class JSObject.

http://codereview.chromium.org/8341008/diff/1/src/ic.cc#newcode1425
src/ic.cc:1425: if (result.is_null()) return Failure::Exception();
RETURN_IF_EMPTY_HANDLE might be clearer.

http://codereview.chromium.org/8341008/diff/1/src/ic.cc#newcode1432
src/ic.cc:1432: && JSArray::cast(*receiver)->AllowsSetElementsLength()) {
I slightly prefer cast then dereference:

Handle<JSArray>::cast(receiver)->Allows....

because it reminds the reader we're in handle code.

http://codereview.chromium.org/8341008/diff/1/src/ic.cc#newcode1463
src/ic.cc:1463: if (receiver->IsJSGlobalProxy()) {
I'm not sure why we patch this site even with --no-use-ic.  Maybe we shouldn't?

http://codereview.chromium.org/8341008/diff/1/src/ic.cc#newcode1519
src/ic.cc:1519: case NORMAL: {
It's minor, but I don't like to have these braces where they're not necessary,
so I remove them when I find them :)

http://codereview.chromium.org/8341008/diff/1/src/ic.cc#newcode1537
src/ic.cc:1537: Handle<AccessorInfo>
callback(AccessorInfo::cast(*callback_object));
Better to have

Handle<AccessorInfo> callback = Handle<AccessorInfo>::cast(callback_object);

here.

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

http://codereview.chromium.org/8341008/diff/1/src/stub-cache.cc#newcode512
src/stub-cache.cc:512: if (probe->IsCode()) return Handle<Code>::cast(probe);
Also space after this line (and below).

[ulan, 2011-10-18 13:51:32]

Please take another look.

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

http://codereview.chromium.org/8341008/diff/1/src/ic.cc#newcode1423
src/ic.cc:1423: HandleScope scope(isolate());
On 2011/10/18 11:39:40, Kevin Millikin wrote:
> This handle scope is not really necessary because of the outer one.  Let's
> remove it.

Done.

http://codereview.chromium.org/8341008/diff/1/src/ic.cc#newcode1424
src/ic.cc:1424: Handle<Object> result = SetElement(receiver, index, value,
strict_mode);
On 2011/10/18 11:39:40, Kevin Millikin wrote:
> As part of this cleanup, we should probably move this SetElement to a static
> member function of class JSObject.

Can we do this in a separate CL after handlification? To minimize possible
conflicts.

http://codereview.chromium.org/8341008/diff/1/src/ic.cc#newcode1425
src/ic.cc:1425: if (result.is_null()) return Failure::Exception();
On 2011/10/18 11:39:40, Kevin Millikin wrote:
> RETURN_IF_EMPTY_HANDLE might be clearer.

Done.

http://codereview.chromium.org/8341008/diff/1/src/ic.cc#newcode1432
src/ic.cc:1432: && JSArray::cast(*receiver)->AllowsSetElementsLength()) {
On 2011/10/18 11:39:40, Kevin Millikin wrote:
> I slightly prefer cast then dereference:
> 
> Handle<JSArray>::cast(receiver)->Allows....
> 
> because it reminds the reader we're in handle code.

Done.

http://codereview.chromium.org/8341008/diff/1/src/ic.cc#newcode1463
src/ic.cc:1463: if (receiver->IsJSGlobalProxy()) {
On 2011/10/18 11:39:40, Kevin Millikin wrote:
> I'm not sure why we patch this site even with --no-use-ic.  Maybe we
shouldn't?

Added TODO.

http://codereview.chromium.org/8341008/diff/1/src/ic.cc#newcode1519
src/ic.cc:1519: case NORMAL: {
On 2011/10/18 11:39:40, Kevin Millikin wrote:
> It's minor, but I don't like to have these braces where they're not necessary,
> so I remove them when I find them :)

Done.

http://codereview.chromium.org/8341008/diff/1/src/ic.cc#newcode1537
src/ic.cc:1537: Handle<AccessorInfo>
callback(AccessorInfo::cast(*callback_object));
On 2011/10/18 11:39:40, Kevin Millikin wrote:
> Better to have
> 
> Handle<AccessorInfo> callback = Handle<AccessorInfo>::cast(callback_object);
> 
> here.

Done.

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

http://codereview.chromium.org/8341008/diff/1/src/stub-cache.cc#newcode512
src/stub-cache.cc:512: if (probe->IsCode()) return Handle<Code>::cast(probe);
On 2011/10/18 11:39:40, Kevin Millikin wrote:
> Also space after this line (and below).

Done.