Move handlified functions from handles.cc to objects.cc

Move handlified functions from handles.cc to objects.cc

BUG=
TEST=

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

[ulan, 2012-01-03 11:11:30]

Please take a look.

This CL is the first in series of changes that convert handlified global
functions to static methods of the corresponding classes.

http://codereview.chromium.org/9008012/diff/1/src/handles.h
File src/handles.h (left):

http://codereview.chromium.org/9008012/diff/1/src/handles.h#oldcode169
src/handles.h:169: 
The functions below are moved to the following classes:

Object: 
    GetPrototype

JSReceiver: 
    SetProperty

JSObject:
    GetIdentityHash	
    DeleteElement
    DeleteProperty
    NormalizeElements
    NormalizeProperties
    PreventExtensions
    SetElement
    SetHiddenProperty
    SetNormalizedProperty
    SetLocalPropertyIgnoreAttributes
    SetLocalPropertyNoThrow
    SetOwnElement
    TransformToFastProperties
    TransitionElementsKind

NumberDictionary:
    Set

http://codereview.chromium.org/9008012/diff/1/src/handles.h#oldcode261
src/handles.h:261: int GetIdentityHash(Handle<JSReceiver> obj);
Made it static method of JSObject, not JSReceiver.

Because the only user (v8::Object::GetIdentityHash) of this function passes
JSObject as an argument and JSReceiver::GetIdentityHash invokes
JSObject::GetIdentityHash in this case.

http://codereview.chromium.org/9008012/diff/1/src/objects.cc
File src/objects.cc (left):

http://codereview.chromium.org/9008012/diff/1/src/objects.cc#oldcode3870
src/objects.cc:3870: MaybeObject* JSReceiver::DeleteProperty(String* name,
DeleteMode mode) {
Placed JSReceiver::DeleteProperty and JSReceiver::DeleteElement after
JSObject::DeleteProperty (line 4034)

http://codereview.chromium.org/9008012/diff/1/src/runtime.cc
File src/runtime.cc (left):

http://codereview.chromium.org/9008012/diff/1/src/runtime.cc#oldcode5141
src/runtime.cc:5141: if (ok->IsRetryAfterGC()) return ok;
Changed it to a handlified function call. Is this correct?

[Kevin Millikin (Chromium), 2012-01-04 13:00:32]

Thanks.  I have some suggestions.

http://codereview.chromium.org/9008012/diff/1/src/api.cc
File src/api.cc (right):

http://codereview.chromium.org/9008012/diff/1/src/api.cc#newcode2837
src/api.cc:2837: i::Handle<i::Object> result = i::Object::GetPrototype(self);
i::Handle<i::Object> result(self->GetPrototype());

http://codereview.chromium.org/9008012/diff/1/src/isolate.cc
File src/isolate.cc (right):

http://codereview.chromium.org/9008012/diff/1/src/isolate.cc#newcode573
src/isolate.cc:573: Handle<JSObject> stackFrame =
factory()->NewJSObject(object_function());
We don't use camelCaseIdentifiers for local variables, and it looks like the
rest of the ones in this function are OK.  Please rename this one to stack_frame
while you're at it.

http://codereview.chromium.org/9008012/diff/1/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/9008012/diff/1/src/objects.cc#newcode748
src/objects.cc:748: Handle<Object> Object::GetPrototype(Handle<Object> obj) {
This seems like a pointless function.  It's not much longer and no less safe to
write

... Handle<Object>(x->GetPrototype()) ...

instead of

... Object::GetPrototype(x) ...

And in declarations one can write:

Handle<Object> y(x->GetPrototype());

instead of

Handle<Object> y = Object::GetPrototype(y);

http://codereview.chromium.org/9008012/diff/1/src/objects.cc#newcode3146
src/objects.cc:3146: void JSObject::SetLocalPropertyNoThrow(Handle<JSObject>
object,
Indentation is wrong.

I don't really like this function, because the name makes it sound like it
somehow swallows exceptions.  Instead, it aborts the process if there is an
exception.

I think it would be clearer to call static
JSObject::SetLocalPropertyIgnoreAttributes at all call sites, and introduce a
macro CHECK_IF_EMPTY_HANDLE that works somewhat like RETURN_IF_EMPTY_HANDLE:

#define CHECK_IF_EMPTY_HANDLE(isolate, call)    \
  do {                                          \
    ASSERT(!isolate->has_pending_exception());  \
    CHECK(!call.is_null());                     \
    CHECK(!isolate->has_pending_exception());   \
  while (false)

(OK, it really CHECKS if not empty handle, to be pedantic.)

http://codereview.chromium.org/9008012/diff/1/src/objects.cc#newcode9486
src/objects.cc:9486: uint32_t index,
Indentation is off here.

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

http://codereview.chromium.org/9008012/diff/1/src/objects.h#newcode1346
src/objects.h:1346: static Handle<Object> SetProperty(Handle<JSReceiver> object,
For these functions that return an empty handle on failure, I wonder if it would
be (a) better and/or (b) really annoying to make them MUST_USE_RESULT.

http://codereview.chromium.org/9008012/diff/1/src/objects.h#newcode1398
src/objects.h:1398: static Handle<Object> SetPrototype(Handle<JSReceiver> obj,
It doesn't look like you added any calls to this (or implemented it), nor did
you remove the corresponding function in handles.

http://codereview.chromium.org/9008012/diff/1/src/objects.h#newcode1633
src/objects.h:1633: Handle<String> key,
Indentation is off here.

http://codereview.chromium.org/9008012/diff/1/src/runtime.cc
File src/runtime.cc (left):

http://codereview.chromium.org/9008012/diff/1/src/runtime.cc#oldcode5141
src/runtime.cc:5141: if (ok->IsRetryAfterGC()) return ok;
On 2012/01/03 11:11:30, ulan wrote:
> Changed it to a handlified function call. Is this correct?

I think it's OK because TransformToFastProperties can only fail with allocation
failures (so no need for RETURN_IF_EMPTY_HANDLE).

On the other hand, it would probably be better to call the raw
TransformToFastProperties anyway.  As written, I don't see a reason for the
handle scope either.  You could just make it:

RUNTIME_FUNCTION(MaybeObject*, Runtime_ToFastProperties) {
  ASSERT(args.length() == 1);
  Object* object = args[0];
  return (object->IsJSObject && !object->IsGlobalObject())
      ? JSObject::cast(object)->TransformToFastProperties(0)
      : object;
}

couldn't you?

http://codereview.chromium.org/9008012/diff/1/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/9008012/diff/1/src/runtime.cc#newcode1400
src/runtime.cc:1400: RETURN_IF_EMPTY_HANDLE(
Some other ways to indent this line seem better than this:

RETURN_IF_EMPTY_HANDLE(
    isolate,
    JSReceiver::SetProperty(object, name, initial_value, mode, kNonStrictMode));

RETURN_IF_EMPTY_HANDLE(isolate,
                       JSReceiver::SetProperty(object, name, initial_value,
                                               mode, kNonStrictMode));

http://codereview.chromium.org/9008012/diff/1/src/runtime.cc#newcode1442
src/runtime.cc:1442: RETURN_IF_EMPTY_HANDLE(
Same: reindent.

http://codereview.chromium.org/9008012/diff/1/src/runtime.cc#newcode1553
src/runtime.cc:1553: RETURN_IF_EMPTY_HANDLE(
Same: reindent.

http://codereview.chromium.org/9008012/diff/1/src/runtime.cc#newcode4063
src/runtime.cc:4063: Handle<Object> prototype = Object::GetPrototype(object);
Handle<Object> prototype(object->GetPrototype());

[ulan, 2012-01-05 11:16:35]

Please take another look. I addressed all comments except the one about
MUST_USE_RESULT (see my comment).

http://codereview.chromium.org/9008012/diff/1/src/api.cc
File src/api.cc (right):

http://codereview.chromium.org/9008012/diff/1/src/api.cc#newcode2837
src/api.cc:2837: i::Handle<i::Object> result = i::Object::GetPrototype(self);
On 2012/01/04 13:00:32, Kevin Millikin wrote:
> i::Handle<i::Object> result(self->GetPrototype());

Done.

http://codereview.chromium.org/9008012/diff/1/src/isolate.cc
File src/isolate.cc (right):

http://codereview.chromium.org/9008012/diff/1/src/isolate.cc#newcode573
src/isolate.cc:573: Handle<JSObject> stackFrame =
factory()->NewJSObject(object_function());
On 2012/01/04 13:00:32, Kevin Millikin wrote:
> We don't use camelCaseIdentifiers for local variables, and it looks like the
> rest of the ones in this function are OK.  Please rename this one to
stack_frame
> while you're at it.

Done.

http://codereview.chromium.org/9008012/diff/1/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/9008012/diff/1/src/objects.cc#newcode748
src/objects.cc:748: Handle<Object> Object::GetPrototype(Handle<Object> obj) {
On 2012/01/04 13:00:32, Kevin Millikin wrote:
> This seems like a pointless function.  It's not much longer and no less safe
to
> write
> 
> ... Handle<Object>(x->GetPrototype()) ...
> 
> instead of
> 
> ... Object::GetPrototype(x) ...
> 
> And in declarations one can write:
> 
> Handle<Object> y(x->GetPrototype());
> 
> instead of
> 
> Handle<Object> y = Object::GetPrototype(y);

Done.

http://codereview.chromium.org/9008012/diff/1/src/objects.cc#newcode3146
src/objects.cc:3146: void JSObject::SetLocalPropertyNoThrow(Handle<JSObject>
object,
On 2012/01/04 13:00:32, Kevin Millikin wrote:
> Indentation is wrong.
> 
> I don't really like this function, because the name makes it sound like it
> somehow swallows exceptions.  Instead, it aborts the process if there is an
> exception.
> 
> I think it would be clearer to call static
> JSObject::SetLocalPropertyIgnoreAttributes at all call sites, and introduce a
> macro CHECK_IF_EMPTY_HANDLE that works somewhat like RETURN_IF_EMPTY_HANDLE:
> 
> #define CHECK_IF_EMPTY_HANDLE(isolate, call)    \
>   do {                                          \
>     ASSERT(!isolate->has_pending_exception());  \
>     CHECK(!call.is_null());                     \
>     CHECK(!isolate->has_pending_exception());   \
>   while (false)
> 
> (OK, it really CHECKS if not empty handle, to be pedantic.)

Done.

http://codereview.chromium.org/9008012/diff/1/src/objects.cc#newcode9486
src/objects.cc:9486: uint32_t index,
On 2012/01/04 13:00:32, Kevin Millikin wrote:
> Indentation is off here.

Done.

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

http://codereview.chromium.org/9008012/diff/1/src/objects.h#newcode1346
src/objects.h:1346: static Handle<Object> SetProperty(Handle<JSReceiver> object,
On 2012/01/04 13:00:32, Kevin Millikin wrote:
> For these functions that return an empty handle on failure, I wonder if it
would
> be (a) better and/or (b) really annoying to make them MUST_USE_RESULT.

(a), I would prefer to get compiler warnings when we forget to check for empty
handle.

I added MUST_USE_RESULT to SetProperty but I am not getting any warnings at
runtime.cc/InstallBuiltin(), which clearly doesn't use the result. Am I doing
something wrong?

http://codereview.chromium.org/9008012/diff/1/src/objects.h#newcode1398
src/objects.h:1398: static Handle<Object> SetPrototype(Handle<JSReceiver> obj,
On 2012/01/04 13:00:32, Kevin Millikin wrote:
> It doesn't look like you added any calls to this (or implemented it), nor did
> you remove the corresponding function in handles.

Deleting for now.

http://codereview.chromium.org/9008012/diff/1/src/objects.h#newcode1633
src/objects.h:1633: Handle<String> key,
On 2012/01/04 13:00:32, Kevin Millikin wrote:
> Indentation is off here.

Done.

http://codereview.chromium.org/9008012/diff/1/src/runtime.cc
File src/runtime.cc (left):

http://codereview.chromium.org/9008012/diff/1/src/runtime.cc#oldcode5141
src/runtime.cc:5141: if (ok->IsRetryAfterGC()) return ok;
Thanks for suggestion, I like it and did the same for ToSlowProperties.

On 2012/01/04 13:00:32, Kevin Millikin wrote:
> On 2012/01/03 11:11:30, ulan wrote:
> > Changed it to a handlified function call. Is this correct?
> 
> I think it's OK because TransformToFastProperties can only fail with
allocation
> failures (so no need for RETURN_IF_EMPTY_HANDLE).
> 
> On the other hand, it would probably be better to call the raw
> TransformToFastProperties anyway.  As written, I don't see a reason for the
> handle scope either.  You could just make it:
> 
> RUNTIME_FUNCTION(MaybeObject*, Runtime_ToFastProperties) {
>   ASSERT(args.length() == 1);
>   Object* object = args[0];
>   return (object->IsJSObject && !object->IsGlobalObject())
>       ? JSObject::cast(object)->TransformToFastProperties(0)
>       : object;
> }
> 
> couldn't you?

http://codereview.chromium.org/9008012/diff/1/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/9008012/diff/1/src/runtime.cc#newcode1400
src/runtime.cc:1400: RETURN_IF_EMPTY_HANDLE(
On 2012/01/04 13:00:32, Kevin Millikin wrote:
> Some other ways to indent this line seem better than this:
> 
> RETURN_IF_EMPTY_HANDLE(
>     isolate,
>     JSReceiver::SetProperty(object, name, initial_value, mode,
kNonStrictMode));
> 
> RETURN_IF_EMPTY_HANDLE(isolate,
>                        JSReceiver::SetProperty(object, name, initial_value,
>                                                mode, kNonStrictMode));

Done.

http://codereview.chromium.org/9008012/diff/1/src/runtime.cc#newcode1442
src/runtime.cc:1442: RETURN_IF_EMPTY_HANDLE(
On 2012/01/04 13:00:32, Kevin Millikin wrote:
> Same: reindent.

Done.

http://codereview.chromium.org/9008012/diff/1/src/runtime.cc#newcode1553
src/runtime.cc:1553: RETURN_IF_EMPTY_HANDLE(
On 2012/01/04 13:00:32, Kevin Millikin wrote:
> Same: reindent.

Done.

http://codereview.chromium.org/9008012/diff/1/src/runtime.cc#newcode4063
src/runtime.cc:4063: Handle<Object> prototype = Object::GetPrototype(object);
On 2012/01/04 13:00:32, Kevin Millikin wrote:
> Handle<Object> prototype(object->GetPrototype());

Done.

http://codereview.chromium.org/9008012/diff/7001/src/factory.cc
File src/factory.cc (right):

http://codereview.chromium.org/9008012/diff/7001/src/factory.cc#newcode754
src/factory.cc:754: CHECK_NOT_EMPTY_HANDLE(isolate(),
isolate() == prototype->GetIsolate(), right?

[Kevin Millikin (Chromium), 2012-01-05 12:59:39]

LGTM, formatting suggestion below.

http://codereview.chromium.org/9008012/diff/7001/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/9008012/diff/7001/src/runtime.cc#newcode1400
src/runtime.cc:1400: RETURN_IF_EMPTY_HANDLE(isolate,
No biggy, I prefer my first suggestion (but you may disagree):

RETURN_IF_EMPTY_HANDLE(
    isolate,
    JSReceiver::SetProperty(object, name, initial_value, mode, kNonStrictMode));

One fewer line, doesn't run to the right as fast (so scales better), and breaks
the line at a better logical place---the commas in the outer
RETURN_IF_EMPTY_HANDLE argument list don't bind as "tightly" as the commas in
the nested SetProperty argument list.

http://codereview.chromium.org/9008012/diff/7001/src/runtime.cc#newcode1443
src/runtime.cc:1443: RETURN_IF_EMPTY_HANDLE(isolate,
Same story:

RETURN_IF_EMPTY_HANDLE(
    isolate,
    JSReceiver::SetProperty(object, name, value, mode, kNonStrictMode));

Here you'll have the kind-of unsightly dangling '(', might as well try to have
it at a place that binds least tightly in the expression.

http://codereview.chromium.org/9008012/diff/7001/src/runtime.cc#newcode1554
src/runtime.cc:1554: RETURN_IF_EMPTY_HANDLE(isolate,
Same story:

RETURN_IF_EMPTY_HANDLE(
    isolate,
    JSReceiver::SetProperty(global, name, value, attributes, kNonStrictMode));

[ulan, 2012-01-05 17:13:17]

Addressed comments, landing.

I will deal with MUST_USE_RESULT in another CL. Is it worth adding an explicit
copy constructor to the Handle class so that warnings work in gcc 4.4.3?

http://codereview.chromium.org/9008012/diff/7001/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/9008012/diff/7001/src/runtime.cc#newcode1400
src/runtime.cc:1400: RETURN_IF_EMPTY_HANDLE(isolate,
On 2012/01/05 12:59:39, Kevin Millikin wrote:
> No biggy, I prefer my first suggestion (but you may disagree):
> 
> RETURN_IF_EMPTY_HANDLE(
>     isolate,
>     JSReceiver::SetProperty(object, name, initial_value, mode,
kNonStrictMode));
> 
> One fewer line, doesn't run to the right as fast (so scales better), and
breaks
> the line at a better logical place---the commas in the outer
> RETURN_IF_EMPTY_HANDLE argument list don't bind as "tightly" as the commas in
> the nested SetProperty argument list.

I fixed the style for RETURN_IF_EMPTY_HANDLE (although SetProperty(...) does not
always fit into one line).

I am leaving CHECK_NOT_EMPTY_HANDLE style unchanged, because fixing it would
just add more new lines.

http://codereview.chromium.org/9008012/diff/7001/src/runtime.cc#newcode1443
src/runtime.cc:1443: RETURN_IF_EMPTY_HANDLE(isolate,
On 2012/01/05 12:59:39, Kevin Millikin wrote:
> Same story:
> 
> RETURN_IF_EMPTY_HANDLE(
>     isolate,
>     JSReceiver::SetProperty(object, name, value, mode, kNonStrictMode));
> 
> Here you'll have the kind-of unsightly dangling '(', might as well try to have
> it at a place that binds least tightly in the expression.

Done.

http://codereview.chromium.org/9008012/diff/7001/src/runtime.cc#newcode1554
src/runtime.cc:1554: RETURN_IF_EMPTY_HANDLE(isolate,
On 2012/01/05 12:59:39, Kevin Millikin wrote:
> Same story:
> 
> RETURN_IF_EMPTY_HANDLE(
>     isolate,
>     JSReceiver::SetProperty(global, name, value, attributes, kNonStrictMode));

Done.