Kiss goodbye to MaybeObject.

Kiss goodbye to MaybeObject.

R=hpayer@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=21086

[Hannes Payer (out of office), 2014-04-30 07:07:06]

LGTM. Cool stuff! I left just nits.

Would you mind renaming all variable and parameter names in all functions you
touched to correspond to our coding style guide ("Function names, variable
names, and
filenames should be descriptive; eschew abbreviation."). I started commenting in
one file but stopped because I spotted these violations in almost every file. We
should use proper names, no abbreviations, e.g., obj -> object, str -> string,
... Since you are already touching that code, it would be awesome to clean that
up.

https://codereview.chromium.org/259173003/diff/1/src/heap-inl.h
File src/heap-inl.h (right):

https://codereview.chromium.org/259173003/diff/1/src/heap-inl.h#newcode134
src/heap-inl.h:134: uint32_t hash_field) {
indent

https://codereview.chromium.org/259173003/diff/1/src/heap-inl.h#newcode406
src/heap-inl.h:406: return false;
Is this necessary here? This case is caught by the default UNREACHABLE before
below.

https://codereview.chromium.org/259173003/diff/1/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/259173003/diff/1/src/heap.cc#newcode2423
src/heap.cc:2423: HeapObject* obj;
obj -> object; c++ google styleguide "Function names, variable names, and
filenames should be descriptive; eschew abbreviation."

https://codereview.chromium.org/259173003/diff/1/src/heap.cc#newcode3606
src/heap.cc:3606: JSObject* js_obj = JSObject::cast(result);
js_obj -> js_object

https://codereview.chromium.org/259173003/diff/1/src/heap.cc#newcode3683
src/heap.cc:3683: JSObject* js_obj;
js_obj -> js_object

https://codereview.chromium.org/259173003/diff/1/src/heap.cc#newcode3984
src/heap.cc:3984: PretenureFlag pretenure) {
indent

https://codereview.chromium.org/259173003/diff/1/src/heap.cc#newcode4026
src/heap.cc:4026: AllocationResult Heap::CopyAndTenureFixedCOWArray(FixedArray*
src) {
src -> source

https://codereview.chromium.org/259173003/diff/1/src/heap.cc#newcode4032
src/heap.cc:4032: HeapObject* obj;
obj -> object

https://codereview.chromium.org/259173003/diff/1/src/heap.cc#newcode4059
src/heap.cc:4059: AllocationResult Heap::CopyFixedArrayWithMap(FixedArray* src,
Map* map) {
src -> source

https://codereview.chromium.org/259173003/diff/1/src/heap.cc#newcode4061
src/heap.cc:4061: HeapObject* obj;
obj -> object

https://codereview.chromium.org/259173003/diff/1/src/heap.cc#newcode4084
src/heap.cc:4084: AllocationResult
Heap::CopyFixedDoubleArrayWithMap(FixedDoubleArray* src,
src -> source

https://codereview.chromium.org/259173003/diff/1/src/heap.cc#newcode4087
src/heap.cc:4087: HeapObject* obj;
obj -> object

https://codereview.chromium.org/259173003/diff/1/src/heap.cc#newcode4100
src/heap.cc:4100: AllocationResult
Heap::CopyConstantPoolArrayWithMap(ConstantPoolArray* src,
src -> source

https://codereview.chromium.org/259173003/diff/1/src/heap.cc#newcode4635
src/heap.cc:4635: case INVALID_SPACE:
Instead of case INVALID_SPACE we could also use default...

https://codereview.chromium.org/259173003/diff/1/src/mark-compact.cc
File src/mark-compact.cc (right):

https://codereview.chromium.org/259173003/diff/1/src/mark-compact.cc#newcode21
src/mark-compact.cc:21: #include "spaces-inl.h"
order:
#include "spaces-inl.h"
#include "stub-cache.h"
#include "sweeper-thread.h"

https://codereview.chromium.org/259173003/diff/1/src/spaces.h
File src/spaces.h (right):

https://codereview.chromium.org/259173003/diff/1/src/spaces.h#newcode1500
src/spaces.h:1500: static inline FreeListNode* cast(Object* maybe) {
maybe -> object

https://codereview.chromium.org/259173003/diff/1/src/v8globals.h
File src/v8globals.h (right):

https://codereview.chromium.org/259173003/diff/1/src/v8globals.h#newcode165
src/v8globals.h:165: INVALID_SPACE,
Can we have a comment here?

[Sven Panne, 2014-04-30 07:44:53]

A few rambling remarks about naming:

I'm not sure if renaming all that stuff is really worth and if it's implied by
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#General_Naming...
at all. It says "Give as descriptive a name as possible, within reason." This is
intentionally vague, and IMHO "src", "dst", "obj" are very common names in CS
and are actually more readable for me than their whole-word counterparts when
used in context. Do we want to rename loop variables from "i" to "index" (or
whatever)? I don't think so. Do we want Hungarian notation ("js_object")? I
don't think so, either. Sensible addenda to the style guide are:

   * Use a name describing the purpose of an entity, not its type.

   * The length of the name of an entity should be proportional to its scope (=>
i, j, k... loop variables, MyComplexGlobalClass, etc.)

[Hannes Payer (out of office), 2014-04-30 07:56:14]

I don't think it is vague, it is very precise "General Naming Rules: Function
names, variable names, and filenames should be descriptive; eschew
abbreviation.".

[Sven Panne, 2014-04-30 08:02:06]

On 2014/04/30 07:56:14, Hannes Payer wrote:
> I don't think it is vague, it is very precise "General Naming Rules: Function
> names, variable names, and filenames should be descriptive; eschew
> abbreviation.".

It *is* vague, otherwise "i", "j", "k", ... would be disallowed, which they are
obviously not.

[Jakob Kummerow, 2014-04-30 09:58:12]

DBC:

I'm with Sven here. When it can reasonably be assumed that everyone (in our team
and beyond) will immediately and unambiguously understand a short name, then
it's fine to keep it: "obj", "src", "dst", "str", "args", etc are all very
common; expanding/renaming them is not worth our time and adds needless clutter.

The style guide even includes a related example:

int num_dns_connections;   // "num" is a widespread convention. Most people know
what "DNS" stands for.

[Hannes Payer (out of office), 2014-04-30 10:39:45]

On 2014/04/30 09:58:12, Jakob wrote:
> DBC:
> 
> I'm with Sven here. When it can reasonably be assumed that everyone (in our
team
> and beyond) will immediately and unambiguously understand a short name, then
> it's fine to keep it: "obj", "src", "dst", "str", "args", etc are all very
> common; expanding/renaming them is not worth our time and adds needless
clutter.
> 
> The style guide even includes a related example:
> 
> int num_dns_connections;   // "num" is a widespread convention. Most people
know
> what "DNS" stands for.

Using e.g. obj and object for "object" results in inconsistency. The style guide
clearly says what to do. Anyhow, I could not care less. Yang, go ahead and do
whatever you think is the right thing.

[Yang, 2014-04-30 12:25:25]

On 2014/04/30 10:39:45, Hannes Payer wrote:
> On 2014/04/30 09:58:12, Jakob wrote:
> > DBC:
> > 
> > I'm with Sven here. When it can reasonably be assumed that everyone (in our
> team
> > and beyond) will immediately and unambiguously understand a short name, then
> > it's fine to keep it: "obj", "src", "dst", "str", "args", etc are all very
> > common; expanding/renaming them is not worth our time and adds needless
> clutter.
> > 
> > The style guide even includes a related example:
> > 
> > int num_dns_connections;   // "num" is a widespread convention. Most people
> know
> > what "DNS" stands for.
> 
> Using e.g. obj and object for "object" results in inconsistency. The style
guide
> clearly says what to do. Anyhow, I could not care less. Yang, go ahead and do
> whatever you think is the right thing.

I'm kind of torn. How far would you go? Change 'dst' into 'destination'? If not,
what's the point in changing 'src' into 'source'?
In any case, I prefer not to do this in this already large CL.

[Yang, 2014-04-30 12:25:33]

https://codereview.chromium.org/259173003/diff/1/src/heap-inl.h
File src/heap-inl.h (right):

https://codereview.chromium.org/259173003/diff/1/src/heap-inl.h#newcode406
src/heap-inl.h:406: return false;
On 2014/04/30 07:07:07, Hannes Payer wrote:
> Is this necessary here? This case is caught by the default UNREACHABLE before
> below.

The compiler would otherwise complain that there is no default case if I don't
list all possible cases of the enum. changed to use default

https://codereview.chromium.org/259173003/diff/1/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/259173003/diff/1/src/heap.cc#newcode4635
src/heap.cc:4635: case INVALID_SPACE:
On 2014/04/30 07:07:07, Hannes Payer wrote:
> Instead of case INVALID_SPACE we could also use default...

Done.

[Yang, 2014-04-30 12:25:38]

Committed patchset #2 manually as r21086 (presubmit successful).

[Sven Panne, 2014-04-30 12:30:51]

https://codereview.chromium.org/259173003/diff/1/src/heap-inl.h
File src/heap-inl.h (right):

https://codereview.chromium.org/259173003/diff/1/src/heap-inl.h#newcode406
src/heap-inl.h:406: return false;
On 2014/04/30 12:25:34, Yang wrote:
> The compiler would otherwise complain that there is no default case if I don't
> list all possible cases of the enum. changed to use default

Already too late, but nevertheless: Please don't use 'default', it makes future
changes much, much harder. Explicitly marking it as unreachable was better.
We've been through this pain in other places several times before...