Thanks for the quick review.
Not a full answer here, just a quick reply on some of the issues you raised:
- I assumed a ResetToBookmark may fail. If I drop that assumption, the code
becomes easier.
- I'm not sure about the scanner state. That /should/ be easy, but it didn't
look that way to me, so I took an admittedly brute-force-ish route of just
saving all the state. If there's a better way, I'd need a hint... :)
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)
On 2015/04/22 16:40:55, marja wrote:
... 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"?
Yes. Cannot bookmark, or !is_lazily_parsed
https://codereview.chromium.org/1102523003/diff/1/src/parser.cc#newcode4064
src/parser.cc:4064: // action: eager parse.
On 2015/04/22 16:40:55, marja wrote:
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.
That'd work as well. I'll change it.
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()) {
On 2015/04/22 16:40:55, marja wrote:
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.
The code certainly gets a lot cleaner if we can predict whether we can
reset or not.
I did this on purpose, to support streaming parsing: When you have an
implementation that looks at the source block-by-block, then it's fully
well possible that you can reset while you're still in the same block,
but you can't if you've progressed to a new block in the mean time.
If we don't want to support this, we can make both API & implementation
simpler by assuming we can always reset if we have successfully set a
bookmark.
Not sure which way we'd want to go...
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#newcode37
src/scanner.cc:37:
On 2015/04/22 16:40:56, marja wrote:
... 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.)
(answered elsewhere:) The idea is to support a stream implementation
that truly streams, where you can reset while you're still in the same
block, but not when you're on a new one.
Maybe we simply shouldn't do this, and make any 'streaming'
implementation hold on to past blocks, at least while they have a
bookmark set?
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_;
On 2015/04/22 16:40:56, marja wrote:
Hmm, why can't we just retroactively recover all these once we have
set the
stream into the right position?
Yeah... I went a bit trial-and-error here and didn't see an /obvious/
way to do this. I didn't see a hard-and-fast rule when content goes into
which literal buffer. Also, using the stream's push back functionality
can create a state that may not be recoverable by merely going to the
old position.
Are there any invariants about the content of the literal buffers? Can I
skip back a bit more, and then read forward until I'm at 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.
