Make Failure inherit from MaybeObject instead of Object.

Make Failure inherit from MaybeObject instead of Object.

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

[Mads Ager (chromium), 2010-10-25 10:29:32]

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.

[Erik Corry, 2010-10-25 13:53:53]

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.