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