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);
Removed the second ToObject call. However I kept the reinterpret_cast
because MaybeObject::To calls ScopeInfo::cast which in turn asserts that
the object in question has a scope_info_map which this one does not yet
have.
On 2011/11/02 16:31:19, Kevin Millikin wrote:
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:
tools/presubmit.py didn't complain. Maybe it should?
On 2011/11/02 16:31:19, Kevin Millikin wrote:
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) {
On 2011/11/02 16:31:19, Kevin Millikin wrote:
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.)
Done.
http://codereview.chromium.org/8352039/diff/8001/src/objects.h#newcode3147
src/objects.h:3147: int NumberOfContextSlots();
Renamed it to ContextLength.
On 2011/11/02 16:31:19, Kevin Millikin wrote:
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();
On 2011/11/02 16:31:19, Kevin Millikin wrote:
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).
Done.
http://codereview.chromium.org/8352039/diff/8001/src/objects.h#newcode3187
src/objects.h:3187: int ContextSlotIndex(String* name, VariableMode*
mode = NULL);
On 2011/11/02 16:31:19, Kevin Millikin wrote:
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.
Done.
http://codereview.chromium.org/8352039/diff/8001/src/objects.h#newcode3209
src/objects.h:3209: #define SCOPE_INFO_NUMERIC_FIELDS(V)
\
Kept the higher order macro but now it generates the Index part of the
name and the default value 0. It's also undef'd now :)
On 2011/11/02 16:31:19, Kevin Millikin wrote:
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 {
On 2011/11/02 16:31:19, Kevin Millikin wrote:
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.
Done.
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);
Done. However AFAICS this is the only HandleScope that can be removed.
But you might want to check as well.
On 2011/11/02 16:31:19, Kevin Millikin wrote:
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) {
On 2011/11/02 16:31:19, Kevin Millikin wrote:
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).
Done.
http://codereview.chromium.org/8352039/diff/8001/src/scopeinfo.cc#newcode50
src/scopeinfo.cc:50: ZoneList<Variable*> variables(32); // 32 is a wild
guess
Will do this in a new CL. Here we are only interested in stack and
context locals. The only other client is
is FunctionInfoListener::SerializeFunctionScope in liveedit.cc which is
only interested in the context locals. So we can do even better here.
On 2011/11/02 16:31:19, Kevin Millikin wrote:
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;
A bit flip because of faulty hardware :P
Done.
On 2011/11/02 16:31:19, Kevin Millikin wrote:
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() {
On 2011/11/02 16:31:19, Kevin Millikin wrote:
I like
return length() > 0 && CallsEvalField::decode(flags());
but maybe you don't.
Done.
http://codereview.chromium.org/8352039/diff/8001/src/scopeinfo.cc#newcode214
src/scopeinfo.cc:214: bool function_name_stack_slot = STACK ==
On 2011/11/02 16:31:19, Kevin Millikin wrote:
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.
Done.
http://codereview.chromium.org/8352039/diff/8001/src/scopeinfo.cc#newcode225
src/scopeinfo.cc:225: bool function_name_context_slot = CONTEXT ==
On 2011/11/02 16:31:19, Kevin Millikin wrote:
Same comment as just above.
Done.
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
I keep that for a CL on its own, because I have other pending CLs that
change scopeinfo.cc.
On 2011/11/02 16:31:19, Kevin Millikin wrote:
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/diff/13001/src/runtime.cc
File src/runtime.cc (right):
http://codereview.chromium.org/8352039/diff/13001/src/runtime.cc#newcode10795
src/runtime.cc:10795: String* name = scope_info->LocalName(i);
Shall I keep the handle here, to be consistent in handlified code?
http://codereview.chromium.org/8352039/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
