Thanks for the thorough review. I addressed all of your comments except for
refactorings that should be done in another CL. I'm a bit concerned about
the
memory usage though. This change now stores the complete scope tree of a
function even though this information is only needed when debugging. And
there
we don't care about performance anyway. So instead I would really consider
what
we talked about earlier: reparsing the source and redoing the variable
resolution + allocation to recreate the scope tree. What do you think?
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);
Doing it explicitly now as it also gives me more control over which
positions I actually record. Also added a cctest for that.
On 2011/10/05 08:43:36, Kevin Millikin wrote:
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);
Removed the local variables and removed them from FunctionLiteral.
On 2011/10/05 08:43:36, Kevin Millikin wrote:
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 {
Almost. The ScopeIterator so far makes a difference about the innermost
function scope (ScopeTypeLocal) and outer function scopes
(ScopeTypeClosure). This would have to be unified if you want to use
Scope::Type. What do you think?
On 2011/10/05 08:43:36, Kevin Millikin wrote:
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
Yes I agree. We will talk about this cleanup later again.
On 2011/10/05 08:43:36, Kevin Millikin wrote:
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)
No we can unify those classes, they only differ slightly in the
functionality they expose.
On 2011/10/05 08:43:36, Kevin Millikin wrote:
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#newcode475
src/scopeinfo.cc:475: return -1;
Yes it is. Using the named constant now.
On 2011/10/05 08:43:36, Kevin Millikin wrote:
-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"
Yes, I totally agree.
On 2011/10/05 08:43:36, Kevin Millikin wrote:
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
Done. But could use more love when cleaning up
ScopeInfo/SerializedScopeInfo.
On 2011/10/05 08:43:36, Kevin Millikin wrote:
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.
Added a description to scopes.h. PTAL there.
On 2011/10/05 08:43:36, Kevin Millikin wrote:
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;
It is set for all scopes during initial parsing, but not necessarily
when deserializing the outer scope chain. There we only get a
SerializedScopeInfo for function and block scopes, but not for with or
catch scopes.
I removed the redundant positions from function literal. It now gets the
positions from the scope and which are always valid.
On 2011/10/05 08:43:36, Kevin Millikin wrote:
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_; }
On 2011/10/05 08:43:36, Kevin Millikin wrote:
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_; }
Done.
http://codereview.chromium.org/7979001/diff/1/src/scopes.h#newcode216
src/scopes.h:216: int SourceEndStatementPos() const { return
source_end_statement_pos_; }
On 2011/10/05 08:43:36, Kevin Millikin wrote:
Likewise:
int end_position() const { return end_position_; }
Done.
http://codereview.chromium.org/7979001/diff/1/src/scopes.h#newcode289
src/scopes.h:289: ZoneList<Scope*>* InnerScopes() { return
&inner_scopes_; }
On 2011/10/05 08:43:36, Kevin Millikin wrote:
ZoneList<Scope*>* inner_scopes() { return &inner_scopes_; }
Done.
http://codereview.chromium.org/7979001/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
