My comments are mainly about the style of objects.h and about the use of
handles
in the ScopeInfo API.
http://codereview.chromium.org/8352039/diff/8001/src/heap.cc
File src/heap.cc (right):
http://codereview.chromium.org/8352039/diff/8001/src/heap.cc#newcode4417
src/heap.cc:4417: maybe_result->ToObject(&result);
No need to call ToObject twice, is there?
This can be more simply be written:
ScopeInfo* scope_info;
MaybeObject* maybe_result = AllocateFixedArray(length, TENURED);
if (!maybe_result->To(&scope_info)) return maybe_result;
scope_info->set_map(scope_info_map());
return scope_info;
http://codereview.chromium.org/8352039/diff/8001/src/objects.h
File src/objects.h (right):
http://codereview.chromium.org/8352039/diff/8001/src/objects.h#newcode3110
src/objects.h:3110:
Presubmit check is not going to like this extra blank line.
http://codereview.chromium.org/8352039/diff/8001/src/objects.h#newcode3111
src/objects.h:3111: static ScopeInfo* cast(Object* object) {
We normally do this this way:
in objects.h:
static inline ScopeInfo* cast(Object* object);
and in objects-inl.h:
CAST_ACCESSOR(ScopeInfo)
(I actually would support moving CAST_ACCESSOR into objects.h and just
using it in the class body, but that's a separate change.)
http://codereview.chromium.org/8352039/diff/8001/src/objects.h#newcode3147
src/objects.h:3147: int NumberOfContextSlots();
I've always found it a bit confusing to count the context header as
"slots". Perhaps we should make this so it does not count the size of
the header; or else count the header size and rename it to something
like ContextLength.
http://codereview.chromium.org/8352039/diff/8001/src/objects.h#newcode3159
src/objects.h:3159: Handle<String> function_name();
There's a strange mix of hacker_style and CamelCase function names in
here. I can't figure out why, that's distracting, so maybe we should
adopt one or the other (hopefully CamelCase).
http://codereview.chromium.org/8352039/diff/8001/src/objects.h#newcode3187
src/objects.h:3187: int ContextSlotIndex(String* name, VariableMode*
mode = NULL);
I'm not thrilled with the default parameter. It does seem to fit the
allowable exception (being used to simulate variable length argument
lists), but it also seems unnecessary.
If you make the caller always pass it, it's only inconvenient at a
handful of sites, and then the lookup functions don't have to check for
mode != NULL.
http://codereview.chromium.org/8352039/diff/8001/src/objects.h#newcode3209
src/objects.h:3209: #define SCOPE_INFO_NUMERIC_FIELDS(V)
\
OK to use a macro to generate these, but there's no need for the
higher-order macro (which isn't undef'd here, kind of scary). It isn't
much worse to have
#define FIELD_ACCESSORS(index, name) \
...
FIELD_ACCESSORS(FLAGS, flags)
FIELD_ACCESSORS(NUM_PARAMETERS, num_parameters)
...
#undef FIELD_ACCESSORS
Where I've taken advantage of the default being 0 for all of them and of
the ability to generate the _INDEX part of the name in the macro.
http://codereview.chromium.org/8352039/diff/8001/src/objects.h#newcode3230
src/objects.h:3230: enum {
Or, you could keep the higher-order macro, and use it here. I'd rename
it and also rename the enum tags to fit the current style guide:
enum {
#define DECL_INDEX(index, name) k##index,
FOR_EACH_NUMERIC_FIELD(DECL_INDEX)
#undef DECL_INDEX
kVariablePartIndex
};
And you might as well make this enum private (since it's not named it
can't really be part of an signature anyway), and also
FunctionVariableInfo, and all the classes defining the bit fields.
http://codereview.chromium.org/8352039/diff/8001/src/profile-generator.cc
File src/profile-generator.cc (right):
http://codereview.chromium.org/8352039/diff/8001/src/profile-generator.cc#newcode2072
src/profile-generator.cc:2072: String* local_name =
*scope_info->context_local_name(i);
Not your code, but while you're cleaning this up I wonder if we should
change the API of the ScopeInfo so that it doesn't use handles.
I don't like handle scopes in raw pointer code if we can avoid them.
From a quick inspection, it seems like almost all of the callers of
local_name, function_name, etc. just dereference the newly-created
handle immediately.
If that's so, it's way, way better to have a raw pointer return type and
make the handful of callers that want a handle have to create one.
Then we can probably get rid of some silly handle scopes, like the one
in this function.
http://codereview.chromium.org/8352039/diff/8001/src/scopeinfo.cc
File src/scopeinfo.cc (right):
http://codereview.chromium.org/8352039/diff/8001/src/scopeinfo.cc#newcode49
src/scopeinfo.cc:49: Handle<ScopeInfo> ScopeInfo::Create(Scope* scope) {
Now that you've removed the template parameter, I think you can make
some other functions non-templated too (Scope::CollectUsedVariables is
the one I'm thinking of).
http://codereview.chromium.org/8352039/diff/8001/src/scopeinfo.cc#newcode50
src/scopeinfo.cc:50: ZoneList<Variable*> variables(32); // 32 is a wild
guess
Again, not your code, but the "wild guess" here seems silly to me. I'd
bet that most variables are used, so we'd get a both a tighter upper
bound, that also guaranteed no need to grow the list, if we just used
the scope's number of variables (parameters, locals, and temps).
http://codereview.chromium.org/8352039/diff/8001/src/scopeinfo.cc#newcode60
src/scopeinfo.cc:60: if (!var->is_used()) continue;
It's not clear how "CollectUsedVariables" can collect one that's unused.
Do you have any idea?
http://codereview.chromium.org/8352039/diff/8001/src/scopeinfo.cc#newcode189
src/scopeinfo.cc:189: bool ScopeInfo::CallsEval() {
I like
return length() > 0 && CallsEvalField::decode(flags());
but maybe you don't.
http://codereview.chromium.org/8352039/diff/8001/src/scopeinfo.cc#newcode214
src/scopeinfo.cc:214: bool function_name_stack_slot = STACK ==
Hmmm, I could see this requiring two tries to read it correctly. Break
at the lower precedence operator:
bool function_name_stack_slot =
STACK == FunctionVariableField::decode(flags());
I sort of like STACK on the right.
http://codereview.chromium.org/8352039/diff/8001/src/scopeinfo.cc#newcode225
src/scopeinfo.cc:225: bool function_name_context_slot = CONTEXT ==
Same comment as just above.
http://codereview.chromium.org/8352039/diff/8001/src/scopeinfo.h
File src/scopeinfo.h (left):
http://codereview.chromium.org/8352039/diff/8001/src/scopeinfo.h#oldcode38
src/scopeinfo.h:38: // ScopeInfo represents information about different
scopes of a source
It's not very nice to make objects.cc bigger than it is, but at some
point we should move the ScopeInfo implementation into objects.cc
because it's declared in objects.h; and at the same time rename this
file context-slot-cache.h.
http://codereview.chromium.org/8352039/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
