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());
http://codereview.chromium.org/9008012/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
