lgtm but see below.
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()); This is found several places. Perhaps the constructor should just take a string and do the cast in the constructor. That way you concentrate the ugliness in one place. 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); This last line is common between both branches and need not be duplicated. http://codereview.chromium.org/5545006/diff/1/src/parser.cc#newcode699 src/parser.cc:699: Missing blank line http://codereview.chromium.org/5545006/diff/1/src/parser.cc#newcode2439 src/parser.cc:2439: && Token::IsCountOp(peek())) { Everywhere else in this file seems to have the && on the previous line. 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: } Can the contents of this first if be put in a non-virtual method that calls the virtual method for the last part? http://codereview.chromium.org/5545006/diff/1/src/preparser-api.cc#newcode68 src/preparser-api.cc:68: } Do we need a PushBackBlock method? This looks slow. Alternatively the ReadBlock method could read overlapping blocks into the buffer so that this doesn't happen much? 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. Positions ... isn't -> Positions ... are not http://codereview.chromium.org/5545006/diff/1/src/scanner-base.cc#newcode512 src/scanner-base.cc:512: has_line_terminator_before_next_ = false; Can't we assert something here to back up the comment? http://codereview.chromium.org/5545006/diff/1/src/scanner-base.h File src/scanner-base.h (right): http://codereview.chromium.org/5545006/diff/1/src/scanner-base.h#newcode102 src/scanner-base.h:102: virtual void PushBack(uc16 character) = 0; Missing blank line http://codereview.chromium.org/5545006/diff/1/src/scanner-base.h#newcode216 src/scanner-base.h:216: explicit Scanner(); You only need this for single-argument constructors. 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#newcode124 src/scanner.cc:124: blanky blanky here and a few more places. 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 utf -> UTF here and in other places. http://codereview.chromium.org/5545006/diff/1/src/scanner.cc#newcode205 src/scanner.cc:205: if ((character & 0x80u) != 0) { if (character > Utf8::kMaxOneByteChar) is nicer because the constant is named http://codereview.chromium.org/5545006/diff/1/src/scanner.cc#newcode206 src/scanner.cc:206: ASSERT((character & 0xC0) == 0x80); (character & Utf8::kMultiByteEncodingMask) == Utf8::kMultiByteEncodingLastChar These constants don't exist yet. http://codereview.chromium.org/5545006/diff/1/src/scanner.cc#newcode210 src/scanner.cc:210: while (buffer[--*cursor] < 0xC0u) { } kMultiByteEncodingFirstChar http://codereview.chromium.org/5545006/diff/1/src/scanner.cc#newcode226 src/scanner.cc:226: ASSERT((character & 0xC0) == 0xC0); Named constants. http://codereview.chromium.org/5545006/diff/1/src/scanner.cc#newcode231 src/scanner.cc:231: // Encode that in a single value Missing full stop. http://codereview.chromium.org/5545006/diff/1/src/scanner.cc#newcode233 src/scanner.cc:233: ((0x3211u) >> (((character - 0xC0) >> 2) & 0xC)) & 0x03; Some places we check whether the unicode character is out of range, but here we just assume that there are no 4 byte encodings. We should be consistent. 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); Missing blank line. http://codereview.chromium.org/5545006/diff/1/src/scanner.h#newcode85 src/scanner.h:85: virtual ~Utf8ToUC16CharacterStream(); Missing blank line http://codereview.chromium.org/5545006/diff/1/src/scanner.h#newcode92 src/scanner.h:92: unsigned raw_data_length_; // Not the number of characters! Measured in bytes? http://codereview.chromium.org/5545006/diff/1/src/scanner.h#newcode142 src/scanner.h:142: explicit JsonScanner(); No explicit. http://codereview.chromium.org/5545006/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
