http://codereview.chromium.org/5545006/diff/1/src/compiler.cc File src/compiler.cc (right):
http://codereview.chromium.org/5545006/diff/1/src/compiler.cc#newcode285 src/compiler.cc:285: Handle<ExternalTwoByteString>::cast(source), 0, source->length()); As discussed offline, let's leave it so that the case is near the test. http://codereview.chromium.org/5545006/diff/1/src/parser.cc File src/parser.cc (right): http://codereview.chromium.org/5545006/diff/1/src/parser.cc#newcode615 src/parser.cc:615: return DoParseProgram(source, in_global_context, &zone_scope); The current code is mandated by the destructor of the stream. The duplicate code is necessary because it needs to live in the scope of different streams. http://codereview.chromium.org/5545006/diff/1/src/parser.cc#newcode2439 src/parser.cc:2439: && Token::IsCountOp(peek())) { I prefer having them on the second line, but let's be consistent for now and do a sweeping change later :) http://codereview.chromium.org/5545006/diff/1/src/preparser-api.cc File src/preparser-api.cc (right): http://codereview.chromium.org/5545006/diff/1/src/preparser-api.cc#newcode60 src/preparser-api.cc:60: } Pushback is a rare operation. I doubt there is anything gained from splitting it up. It would also require changing the UC16CharacterStream base class, which doesn't have a non-virtual PushBack method, and there isn't any generic way to handle it with the current design. I have redesigned the code to have a pushback scratch area before the buffer, so you can push back characters before the currently read block. If that fails, it falls back on pushing everything back and rereading a block from that position. The whole interface should probably be reconsidered. Buffering doesn't play well with having pushback int the stream. http://codereview.chromium.org/5545006/diff/1/src/preparser-api.cc#newcode68 src/preparser-api.cc:68: } The interface should be reconsidered. It might not be perfect for what we are going to use it for. Perhaps it should drop the pushback, leaving it to the reader to handle pushbacks. As it is now, a real streaming source would have to implement its own pushback buffer too. http://codereview.chromium.org/5545006/diff/1/src/scanner-base.cc File src/scanner-base.cc (right): http://codereview.chromium.org/5545006/diff/1/src/scanner-base.cc#newcode504 src/scanner-base.cc:504: // Positions inside the lookahead token isn't supported. On 2010/12/07 12:27:30, Erik Corry wrote:
Positions ... isn't -> Positions ... are not
Done. http://codereview.chromium.org/5545006/diff/1/src/scanner-base.cc#newcode512 src/scanner-base.cc:512: has_line_terminator_before_next_ = false; We can't know whether there is a newline before, since we skip reading the characters. The only option is to check that the next token isn't one that cares about a newline before it. But the requirement is generally about newlines after a token that cares, and not recognizable on the next token. I'll be very specific and require the next token to be '}'. http://codereview.chromium.org/5545006/diff/1/src/scanner.cc File src/scanner.cc (right): http://codereview.chromium.org/5545006/diff/1/src/scanner.cc#newcode201 src/scanner.cc:201: // Move the cursor back to point at the preceding utf-8 character start Fixed. I hope. http://codereview.chromium.org/5545006/diff/1/src/scanner.cc#newcode205 src/scanner.cc:205: if ((character & 0x80u) != 0) { On 2010/12/07 12:27:30, Erik Corry wrote:
if (character > Utf8::kMaxOneByteChar) is nicer because the constant is named
Done. http://codereview.chromium.org/5545006/diff/1/src/scanner.cc#newcode206 src/scanner.cc:206: ASSERT((character & 0xC0) == 0x80); Added functions, but not to unibrow::Utf8. I'll consider moving them later if they turn out to be generally useful, http://codereview.chromium.org/5545006/diff/1/src/scanner.cc#newcode210 src/scanner.cc:210: while (buffer[--*cursor] < 0xC0u) { } IsUtf8MultiCharacterFollower(buffer[--*cursor]) http://codereview.chromium.org/5545006/diff/1/src/scanner.cc#newcode226 src/scanner.cc:226: ASSERT((character & 0xC0) == 0xC0); On 2010/12/07 12:27:30, Erik Corry wrote:
Named constants.
Done. http://codereview.chromium.org/5545006/diff/1/src/scanner.cc#newcode231 src/scanner.cc:231: // Encode that in a single value On 2010/12/07 12:27:30, Erik Corry wrote:
Missing full stop.
Done. http://codereview.chromium.org/5545006/diff/1/src/scanner.cc#newcode233 src/scanner.cc:233: ((0x3211u) >> (((character - 0xC0) >> 2) & 0xC)) & 0x03; It works with four-byte encodings as well. "additional bytes" is in addition to the first byte that we already read. http://codereview.chromium.org/5545006/diff/1/src/scanner.h File src/scanner.h (right): http://codereview.chromium.org/5545006/diff/1/src/scanner.h#newcode46 src/scanner.h:46: virtual void PushBack(uc16 character); On 2010/12/07 12:27:30, Erik Corry wrote:
Missing blank line.
Done. http://codereview.chromium.org/5545006/diff/1/src/scanner.h#newcode85 src/scanner.h:85: virtual ~Utf8ToUC16CharacterStream(); On 2010/12/07 12:27:30, Erik Corry wrote:
Missing blank line
Done. http://codereview.chromium.org/5545006/diff/1/src/scanner.h#newcode92 src/scanner.h:92: unsigned raw_data_length_; // Not the number of characters! Yes. Reworded to say so. http://codereview.chromium.org/5545006/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
