http://codereview.chromium.org/3970005/diff/36001/18002 File src/accessors.cc (right):
http://codereview.chromium.org/3970005/diff/36001/18002#newcode446 src/accessors.cc:446: { MaybeObject* t = Heap::AllocateFunctionPrototype(function); On 2010/10/25 10:29:32, Mads Ager wrote:
Why |t|? We should use a more telling name?
Changed to maybe_prototype and similar names. http://codereview.chromium.org/3970005/diff/36001/18004 File src/api.cc (right): http://codereview.chromium.org/3970005/diff/36001/18004#newcode2548 src/api.cc:2548: ASSERT(!maybe_property->IsFailure()); On 2010/10/25 10:29:32, Mads Ager wrote:
Use the unchecked ToObject and remove assertion from here?
Done. http://codereview.chromium.org/3970005/diff/36001/18004#newcode2571 src/api.cc:2571: ASSERT(!maybe_property->IsFailure()); On 2010/10/25 10:29:32, Mads Ager wrote:
Use the uncheck ToObject and remove assertion from here?
Done. http://codereview.chromium.org/3970005/diff/36001/18006 File src/arm/stub-cache-arm.cc (right): http://codereview.chromium.org/3970005/diff/36001/18006#newcode1783 src/arm/stub-cache-arm.cc:1783: if (result->IsFailure()) return result; On 2010/10/25 10:29:32, Mads Ager wrote:
I would use the extra variable and keep the pattern of
if (!maybe_result->ToObject(&result)) return maybe_result; if (!result->IsUndefined()) return result; ...
Done. http://codereview.chromium.org/3970005/diff/36001/18006#newcode1992 src/arm/stub-cache-arm.cc:1992: if (result->IsFailure()) return result; On 2010/10/25 10:29:32, Mads Ager wrote:
As above, I would have another variable and use the same pattern for
failure
checking as everywhere else.
Done. http://codereview.chromium.org/3970005/diff/36001/18007 File src/bootstrapper.cc (right): http://codereview.chromium.org/3970005/diff/36001/18007#newcode1372 src/bootstrapper.cc:1372: ASSERT(!maybe->IsFailure()); On 2010/10/25 10:29:32, Mads Ager wrote:
Use unchecked ToObject and get rid of assert here.
Done. http://codereview.chromium.org/3970005/diff/36001/18013 File src/execution.cc (right): http://codereview.chromium.org/3970005/diff/36001/18013#newcode175 src/execution.cc:175: // TODO(lrn): Bug 617. We should use the default function here, not the On 2010/10/25 10:29:32, Mads Ager wrote:
TODO(617): We ...
Does this make tests fail? If it doesn't we should see if we can
generate one
that fails and put it under the bugs testing directory.
Done. http://codereview.chromium.org/3970005/diff/36001/18024 File src/ia32/stub-cache-ia32.cc (right): http://codereview.chromium.org/3970005/diff/36001/18024#newcode2086 src/ia32/stub-cache-ia32.cc:2086: if (result->IsFailure()) return result; On 2010/10/25 10:29:32, Mads Ager wrote:
Stick to the pattern?
if (!maybe_result->ToObject(&result)) return maybe_result;
Done. http://codereview.chromium.org/3970005/diff/36001/18024#newcode2308 src/ia32/stub-cache-ia32.cc:2308: if (result->IsFailure()) return result; On 2010/10/25 10:29:32, Mads Ager wrote:
Stick to the pattern?
Done. http://codereview.chromium.org/3970005/diff/36001/18034 File src/objects-inl.h (right): http://codereview.chromium.org/3970005/diff/36001/18034#newcode686 src/objects-inl.h:686: bool MaybeObject::IsTheHole() { On 2010/10/25 10:29:32, Mads Ager wrote:
Could you group all the MaybeObject methods please?
Done. http://codereview.chromium.org/3970005/diff/36001/18034#newcode3374 src/objects-inl.h:3374: ASSERT(!maybe->IsFailure()); On 2010/10/25 10:29:32, Mads Ager wrote:
Use the unchecked ToObject method and remove assert from here.
Done. http://codereview.chromium.org/3970005/diff/36001/18036 File src/objects.h (right): http://codereview.chromium.org/3970005/diff/36001/18036#newcode592 src/objects.h:592: inline bool IsOutOfMemoryFailure(); On 2010/10/25 10:29:32, Mads Ager wrote:
Remove "Failure" from the name (or add it to the other ones for
consistency as
well).
Done. http://codereview.chromium.org/3970005/diff/36001/18036#newcode600 src/objects.h:600: inline Object* ToObjectAndAssert() { On 2010/10/25 10:29:32, Mads Ager wrote:
How about UncheckedToObject or ToObjectUnchecked?
I used ToObjectUnchecked and added ToObjectChecked for use in tests (uses CHECK instead of ASSERT). http://codereview.chromium.org/3970005/diff/36001/18036#newcode725 src/objects.h:725: MUST_USE_RESULT MaybeObject* GetProperty(Object* receiver, On 2010/10/25 10:29:32, Mads Ager wrote:
Indentation.
Done. http://codereview.chromium.org/3970005/diff/36001/18036#newcode1725 src/objects.h:1725: DeleteMode mode); On 2010/10/25 10:29:32, Mads Ager wrote:
Indentation.
Done. http://codereview.chromium.org/3970005/diff/36001/18036#newcode3241 src/objects.h:3241: NormalizedMapSharingMode sharing); You missed this one, you slacker! :-) http://codereview.chromium.org/3970005/diff/36001/18036#newcode4045 src/objects.h:4045: maybe->ToObject(&answer); On 2010/10/25 10:29:32, Mads Ager wrote:
Replace this ToObject with the unchecked ToObject and remove the
assert above
since it is part of the unchecked ToObject.
Done. http://codereview.chromium.org/3970005/diff/36001/18039 File src/runtime.cc (right): http://codereview.chromium.org/3970005/diff/36001/18039#newcode3325 src/runtime.cc:3325: MaybeObject* res = Heap::AllocateStringFromAscii(CStrVector(str)); On 2010/10/25 10:29:32, Mads Ager wrote:
res -> result
Here and in the methods below.
Done. http://codereview.chromium.org/3970005/diff/36001/18039#newcode10087 src/runtime.cc:10087: &pending_exception); On 2010/10/25 10:29:32, Mads Ager wrote:
Revert accidental edit. :)
Done. http://codereview.chromium.org/3970005/diff/36001/18041 File src/serialize.cc (right): http://codereview.chromium.org/3970005/diff/36001/18041#newcode560 src/serialize.cc:560: new_allocation = reinterpret_cast<NewSpace*>(space)->AllocateRaw(size)-> On 2010/10/25 10:29:32, Mads Ager wrote:
Introduce a local variables for the space instead?
NewSpace* new_space = reinterpret_cast<NewSpace*>(space); new_allocation = new_space->AllocateRaw(size)->ToObjectAndAssert();
?
Done. http://codereview.chromium.org/3970005/diff/36001/18041#newcode563 src/serialize.cc:563: new_allocation = reinterpret_cast<PagedSpace*>(space)->AllocateRaw(size)-> On 2010/10/25 10:29:32, Mads Ager wrote:
Ditto.
Done. http://codereview.chromium.org/3970005/diff/36001/18045 File src/stub-cache.cc (right): http://codereview.chromium.org/3970005/diff/36001/18045#newcode1325 src/stub-cache.cc:1325: CodeCreateEvent(Logger::KEYED_LOAD_IC_TAG, On 2010/10/25 10:29:32, Mads Ager wrote:
This should fit on the line above as in the other cases?
Done. http://codereview.chromium.org/3970005/diff/36001/18053 File src/x64/stub-cache-x64.cc (right): http://codereview.chromium.org/3970005/diff/36001/18053#newcode917 src/x64/stub-cache-x64.cc:917: if (result->IsFailure()) return result; On 2010/10/25 10:29:32, Mads Ager wrote:
Follow the pattern and use !maybe_result->ToObject(&result)?
Done. http://codereview.chromium.org/3970005/diff/36001/18053#newcode1780 src/x64/stub-cache-x64.cc:1780: if (result->IsFailure()) return result; On 2010/10/25 10:29:32, Mads Ager wrote:
Ditto.
Done. http://codereview.chromium.org/3970005/diff/36001/18056 File test/cctest/test-assembler-arm.cc (right): http://codereview.chromium.org/3970005/diff/36001/18056#newcode74 test/cctest/test-assembler-arm.cc:74: Handle<Object>(Heap::undefined_value()))-> On 2010/10/25 10:29:32, Mads Ager wrote:
ARGH. :)
Please move either Heap::CreateCode call to a new line if that is
enough. If
not, move the arguments to new lines with four space indent.
Same for the edits below.
Done. http://codereview.chromium.org/3970005/diff/36001/18057 File test/cctest/test-assembler-ia32.cc (right): http://codereview.chromium.org/3970005/diff/36001/18057#newcode74 test/cctest/test-assembler-ia32.cc:74: Handle<Object>(Heap::undefined_value()))-> On 2010/10/25 10:29:32, Mads Ager wrote:
Please reformat all these.
Done. http://codereview.chromium.org/3970005/diff/36001/18060 File test/cctest/test-heap.cc (right): http://codereview.chromium.org/3970005/diff/36001/18060#newcode81 test/cctest/test-heap.cc:81: Handle<Object>(Heap::undefined_value()))-> On 2010/10/25 10:29:32, Mads Ager wrote:
reformat
Done. http://codereview.chromium.org/3970005/diff/36001/18060#newcode96 test/cctest/test-heap.cc:96: ToObjectAndAssert(); On 2010/10/25 10:29:32, Mads Ager wrote:
reformat
Done. http://codereview.chromium.org/3970005/diff/36001/18060#newcode142 test/cctest/test-heap.cc:142: value = Heap::NumberFromUint32(static_cast<uint32_t>(Smi::kMaxValue) + 1)-> On 2010/10/25 10:29:32, Mads Ager wrote:
Reformat. reinterpret_cast on a separate line for instance.
Done. http://codereview.chromium.org/3970005/diff/36001/18060#newcode214 test/cctest/test-heap.cc:214: Top::context()->global()->SetProperty(*name, *function, NONE)-> On 2010/10/25 10:29:32, Mads Ager wrote:
Reformat, please.
Done. http://codereview.chromium.org/3970005/diff/36001/18060#newcode239 test/cctest/test-heap.cc:239: Top::context()->global()->SetProperty(*obj_name, *obj, NONE)-> On 2010/10/25 10:29:32, Mads Ager wrote:
Reformat please. Extract the global first?
Done. http://codereview.chromium.org/3970005/diff/36001/18060#newcode251 test/cctest/test-heap.cc:251: JSObject::cast(Top::context()->global()->GetProperty(*obj_name)-> On 2010/10/25 10:29:32, Mads Ager wrote:
Reformat please. Similarly for the rest of this file.
Done. http://codereview.chromium.org/3970005/diff/36001/18061 File test/cctest/test-mark-compact.cc (right): http://codereview.chromium.org/3970005/diff/36001/18061#newcode170 test/cctest/test-mark-compact.cc:170: mapp = Heap::AllocateMap(JS_OBJECT_TYPE, JSObject::kHeaderSize)-> On 2010/10/25 10:29:32, Mads Ager wrote:
Reformat.
Done. http://codereview.chromium.org/3970005/diff/36001/18061#newcode186 test/cctest/test-mark-compact.cc:186: Top::context()->global()->SetProperty(func_name, function, NONE)-> On 2010/10/25 10:29:32, Mads Ager wrote:
Reformat. Please reformat the entire file to not break the lines like
this. Done. http://codereview.chromium.org/3970005/show -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
