Generally LG, but I think this can be simpler - either that or I'm missing
something (pls tell me what I'm missing). :)
https://codereview.chromium.org/1102523003/diff/1/src/parser.cc
File src/parser.cc (right):
https://codereview.chromium.org/1102523003/diff/1/src/parser.cc#newcode4057
src/parser.cc:4057: // condition: is_lazily_parsed (but none of the
conditions below)
... is this the same as "couldn't trial parse because cannot set
bookmark" or are there some othere reasons why we "have not tried trial
parsing"?
https://codereview.chromium.org/1102523003/diff/1/src/parser.cc#newcode4064
src/parser.cc:4064: // action: eager parse.
The logic sounds correct, but... isn't there a simpler way to express
it? I'm assuming we can know early that we cannot trial parse (and that
the only reason for that is not being able to set the bookmark).
So after we have trial parsed (and we know we can successfully do it),
HasBeenReset and HasBeenSet are complements, right, the first means
"trial aborted" and the latter "trial finished"?
How about:
if (is_lazily_parsed) {
if (bookmark.Set()) { // can set bookmarks
Skip(..); // trial
if (trial parsing aborted) {
is_lazily_parsed = false;
} // else: trial parsing finished, we're done
} else {
Skip(...); // normal lazy parsing without bookmark
}
}
if (!is_lazily_parsed) {
ParseEager(..);
}
That puts the trial parsing logic into the same control flow, so we
don't need to try to figure out in retrospect whether we even tried
trial parsing or not.
https://codereview.chromium.org/1102523003/diff/1/src/parser.cc#newcode4068
src/parser.cc:4068: // But this will cause the compiler to actually
compile it.
... we might want to use the non-hacky way of saying "please compile
this" already in this CL...
https://codereview.chromium.org/1102523003/diff/1/src/preparser.cc
File src/preparser.cc (right):
https://codereview.chromium.org/1102523003/diff/1/src/preparser.cc#newcode246
src/preparser.cc:246: if (count_lines++ > 200 && bookmark->CanReset()) {
Hmm, I find this is quite a late point to check whether we can reset the
bookmark... (and my other comments were also assuming we can know it
pretty early). The code might get cleaner if we can know that early. Is
there some reason why we couldn't? So here we'd only have the bookmark
in those cases where we can reset.
https://codereview.chromium.org/1102523003/diff/1/src/scanner-character-streams.h
File src/scanner-character-streams.h (right):
https://codereview.chromium.org/1102523003/diff/1/src/scanner-character-streams.h#newcode85
src/scanner-character-streams.h:85: class ExternalStreamingStream :
public BufferedUtf16CharacterStream {
Pls add a todo here to implement bookmarking for this stream too; it
should be easy, since we can just ask the embedder to give us data from
a given position. (That might require transmitting the position in the
API..)
https://codereview.chromium.org/1102523003/diff/1/src/scanner.cc
File src/scanner.cc (right):
https://codereview.chromium.org/1102523003/diff/1/src/scanner.cc#newcode34
src/scanner.cc:34: bool Utf16CharacterStream::CanResetToBookmark() {
return false; }
I don't get why we need CanResetToBookmark; if SetBookmark returns true,
we can assume that it successfully set the bookmark and can later
successfully reset to it, no?
https://codereview.chromium.org/1102523003/diff/1/src/scanner.cc#newcode35
src/scanner.cc:35: bool Utf16CharacterStream::ResetToBookmark() { return
false; }
Also here, if we successfully set a bookmark, I'd assume we can reset to
it, and this function has no way of saying "ha ha, we actually cannot"
and returning false.
https://codereview.chromium.org/1102523003/diff/1/src/scanner.cc#newcode37
src/scanner.cc:37:
... or is there some logic here like "this bookmark has already been set
to something and thus we cannot set it to something else"?
(But that could be expressed in a more direct way.)
https://codereview.chromium.org/1102523003/diff/1/src/scanner.h
File src/scanner.h (right):
https://codereview.chromium.org/1102523003/diff/1/src/scanner.h#newcode765
src/scanner.h:765: LiteralBuffer bookmark_raw_literal_;
Hmm, why can't we just retroactively recover all these once we have set
the stream into the right position?
https://codereview.chromium.org/1102523003/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
