I like the approach. I think it's clear that both ScopeInfo and the
ScopeIterator used by the debugger could be cleaned up. Many of my
comments are
tracking suggestions for future cleanup.
I did not look closely at the ScopeIterator yet, I'm doing that now.
http://codereview.chromium.org/7979001/diff/1/src/parser.cc
File src/parser.cc (right):
http://codereview.chromium.org/7979001/diff/1/src/parser.cc#newcode474
src/parser.cc:474:
scope->SetSourceBegStatementPos(parser_->scanner().location().beg_pos);
I'm leery of setting this implicitly. It's too easy for someone who
just wants to modify the parser to misunderstand what's going on.
If we commit this, we must have cctests that will break if the wrong
positions are recorded.
It's a bit less convenient, but better, to have explicit calls in the
parser to set the positions.
http://codereview.chromium.org/7979001/diff/1/src/parser.cc#newcode3889
src/parser.cc:3889: scope->SetSourceBegStatementPos(start_pos);
It's a bit warty that some scopes have positions set explicitly and some
implicitly.
I recommend always doing it explicitly as described above.
If it should be implicit, then it should also be that way here (e.g., by
having the LexicalScope constructor and destructor set the start and end
positions).
This code can also be improved. Since the scope records start/end
positions, we don't really need redundant local variables for them.
We also might get away with not passing them as extra parameters to
FunctionLiteral (or even storing them in FunctionLiteral) since they're
redundantly on the scope.
http://codereview.chromium.org/7979001/diff/1/src/runtime.cc
File src/runtime.cc (right):
http://codereview.chromium.org/7979001/diff/1/src/runtime.cc#newcode10934
src/runtime.cc:10934: enum ScopeType {
Can this be the same as Scope::Type?
http://codereview.chromium.org/7979001/diff/1/src/scopeinfo.cc
File src/scopeinfo.cc (right):
http://codereview.chromium.org/7979001/diff/1/src/scopeinfo.cc#newcode164
src/scopeinfo.cc:164: // - calls eval boolean flag
This is a separate change, but this class should be cleaned up.
(1) The encoding should be represent in code (with something other than
the magic number below), and not just in this comment. I suggest named
constants like all the other HeapObject layouts.
(2) It's a small improvement, but we can pack the flags and type into a
single pointer.
(3) The streaming interface of the Read/Write functions is just clumsy
and doesn't really seem necessary. A straight random access interface
makes it more clear what's going on, I think.
(4) Related to 3, it's probably simplest to have a fixed part up front,
and all the variable sized stuff at the end. This should give the same
performance with a much simpler layout and implementation.
http://codereview.chromium.org/7979001/diff/1/src/scopeinfo.cc#newcode262
src/scopeinfo.cc:262:
ScopeInfo<Allocator>::ScopeInfo(SerializedScopeInfo* data)
Also, for future refactoring. Do we need the ability to construct a
ScopeInfo from a SerializedScopeInfo? (Do we need ScopeInfo at all?)
http://codereview.chromium.org/7979001/diff/1/src/scopeinfo.cc#newcode462
src/scopeinfo.cc:462: p = ReadInt(p, &type);
Future refactoring: if you decide to keep the streaming interface, the
default for Read is wrong. It should be:
Scope::Type type = ReadInt(&p);
So you can just write here:
return ReadInt(&p);
http://codereview.chromium.org/7979001/diff/1/src/scopeinfo.cc#newcode475
src/scopeinfo.cc:475: return -1;
-1 is RelocInfo::kNoPosition?
http://codereview.chromium.org/7979001/diff/1/src/scopeinfo.h
File src/scopeinfo.h (right):
http://codereview.chromium.org/7979001/diff/1/src/scopeinfo.h#newcode34
src/scopeinfo.h:34: #include "scopes.h"
Maybe we should avoid this? It's not yet a circularity, but it risks
one and they cause us a lot of headaches. The Scope and ScopeInfo are
almost independent except for this enum. I think I'd rather have a
ScopeType with all the other enums in v8globals.h so the classes are not
coupled (unless you can convince me otherwise :))
http://codereview.chromium.org/7979001/diff/1/src/scopeinfo.h#newcode39
src/scopeinfo.h:39: // Scope information represents information about a
functions's
This comment seems out of date. And the Historical note is unnecessary
(and not entirely true).
http://codereview.chromium.org/7979001/diff/1/src/scopeinfo.h#newcode132
src/scopeinfo.h:132: // Start position of this scope in the source
string.
I think we want a crisp characterization of start and end positions,
either as a comment in the scopes or in the parser where we set it, or
both. Is it the position of the curly brace? Of the first/last
character inside the curlies?
http://codereview.chromium.org/7979001/diff/1/src/scopes.cc
File src/scopes.cc (right):
http://codereview.chromium.org/7979001/diff/1/src/scopes.cc#newcode215
src/scopes.cc:215: source_beg_statement_pos_ = RelocInfo::kNoPosition;
You could consider setting positions for all scopes. On the one hand,
and invalid position clearly says we don't care and is better than an
incorrect one.
On the other hand, it could be useful to have in general (e.g., to
remove the redundant positions from function literals and so forth).
http://codereview.chromium.org/7979001/diff/1/src/scopes.h
File src/scopes.h (right):
http://codereview.chromium.org/7979001/diff/1/src/scopes.h#newcode210
src/scopes.h:210: int SourceBegStatementPos() const { return
source_beg_statement_pos_; }
I don't like the name :)
First, it doesn't follow the hacker_style naming convention for simple
accessors from the style guide, and it doesn't follow the API naming
convention for the rest of the class.
Second, abbreviations like "beg" and "pos" don't read very well. And
"Source" and "Statement" are just noise words, though should be implicit
unless there's some other kind of positions we care about.
Suggestion:
int start_position() const { return start_position_; }
http://codereview.chromium.org/7979001/diff/1/src/scopes.h#newcode216
src/scopes.h:216: int SourceEndStatementPos() const { return
source_end_statement_pos_; }
Likewise:
int end_position() const { return end_position_; }
http://codereview.chromium.org/7979001/diff/1/src/scopes.h#newcode289
src/scopes.h:289: ZoneList<Scope*>* InnerScopes() { return
&inner_scopes_; }
ZoneList<Scope*>* inner_scopes() { return &inner_scopes_; }
http://codereview.chromium.org/7979001/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
