LGTM with these comments addressed. I really dislike the name |t| for all of the MaybeObject pointers. In some places you have:
MaybeObject* maybe_result = ... Object* result; if (!maybe_result->ToObject(&result)) return maybe_result; That is what it ideally should look like in all the places. 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); Why |t|? We should use a more telling name? 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()); Use the unchecked ToObject and remove assertion from here? http://codereview.chromium.org/3970005/diff/36001/18004#newcode2571 src/api.cc:2571: ASSERT(!maybe_property->IsFailure()); Use the uncheck ToObject and remove assertion from here? 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; I would use the extra variable and keep the pattern of if (!maybe_result->ToObject(&result)) return maybe_result; if (!result->IsUndefined()) return result; ... http://codereview.chromium.org/3970005/diff/36001/18006#newcode1992 src/arm/stub-cache-arm.cc:1992: if (result->IsFailure()) return result; As above, I would have another variable and use the same pattern for failure checking as everywhere else. 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()); Use unchecked ToObject and get rid of assert here. 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 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. 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; Stick to the pattern? if (!maybe_result->ToObject(&result)) return maybe_result; http://codereview.chromium.org/3970005/diff/36001/18024#newcode2308 src/ia32/stub-cache-ia32.cc:2308: if (result->IsFailure()) return result; Stick to the pattern? 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() { Could you group all the MaybeObject methods please? http://codereview.chromium.org/3970005/diff/36001/18034#newcode3374 src/objects-inl.h:3374: ASSERT(!maybe->IsFailure()); Use the unchecked ToObject method and remove assert from here. 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(); Remove "Failure" from the name (or add it to the other ones for consistency as well). http://codereview.chromium.org/3970005/diff/36001/18036#newcode600 src/objects.h:600: inline Object* ToObjectAndAssert() { How about UncheckedToObject or ToObjectUnchecked? http://codereview.chromium.org/3970005/diff/36001/18036#newcode725 src/objects.h:725: MUST_USE_RESULT MaybeObject* GetProperty(Object* receiver, Indentation. http://codereview.chromium.org/3970005/diff/36001/18036#newcode1725 src/objects.h:1725: DeleteMode mode); Indentation. http://codereview.chromium.org/3970005/diff/36001/18036#newcode4045 src/objects.h:4045: maybe->ToObject(&answer); Replace this ToObject with the unchecked ToObject and remove the assert above since it is part of the unchecked ToObject. 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)); res -> result Here and in the methods below. http://codereview.chromium.org/3970005/diff/36001/18039#newcode10087 src/runtime.cc:10087: &pending_exception); Revert accidental edit. :) 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)-> Introduce a local variables for the space instead? NewSpace* new_space = reinterpret_cast<NewSpace*>(space); new_allocation = new_space->AllocateRaw(size)->ToObjectAndAssert(); ? http://codereview.chromium.org/3970005/diff/36001/18041#newcode563 src/serialize.cc:563: new_allocation = reinterpret_cast<PagedSpace*>(space)->AllocateRaw(size)-> Ditto. 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, This should fit on the line above as in the other cases? 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; Follow the pattern and use !maybe_result->ToObject(&result)? http://codereview.chromium.org/3970005/diff/36001/18053#newcode1780 src/x64/stub-cache-x64.cc:1780: if (result->IsFailure()) return result; Ditto. 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()))-> 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. 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()))-> Please reformat all these. http://codereview.chromium.org/3970005/diff/36001/18059 File test/cctest/test-disasm-ia32.cc (right): http://codereview.chromium.org/3970005/diff/36001/18059#newcode443 test/cctest/test-disasm-ia32.cc:443: Handle<Object>(Heap::undefined_value()))-> Please reformat. 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()))-> reformat http://codereview.chromium.org/3970005/diff/36001/18060#newcode96 test/cctest/test-heap.cc:96: ToObjectAndAssert(); reformat 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)-> Reformat. reinterpret_cast on a separate line for instance. http://codereview.chromium.org/3970005/diff/36001/18060#newcode214 test/cctest/test-heap.cc:214: Top::context()->global()->SetProperty(*name, *function, NONE)-> Reformat, please. http://codereview.chromium.org/3970005/diff/36001/18060#newcode239 test/cctest/test-heap.cc:239: Top::context()->global()->SetProperty(*obj_name, *obj, NONE)-> Reformat please. Extract the global first? http://codereview.chromium.org/3970005/diff/36001/18060#newcode251 test/cctest/test-heap.cc:251: JSObject::cast(Top::context()->global()->GetProperty(*obj_name)-> Reformat please. Similarly for the rest of this file. 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)-> Reformat. http://codereview.chromium.org/3970005/diff/36001/18061#newcode186 test/cctest/test-mark-compact.cc:186: Top::context()->global()->SetProperty(func_name, function, NONE)-> Reformat. Please reformat the entire file to not break the lines like this. http://codereview.chromium.org/3970005/show -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
