Lasse, thanks for doing the review. I like all of your comments and improvements.
http://codereview.chromium.org/7241023/diff/5001/src/json-parser.h File src/json-parser.h (right): http://codereview.chromium.org/7241023/diff/5001/src/json-parser.h#newcode113 src/json-parser.h:113: Handle<String> SlowScanJsonString(Handle<String> prefix, int start, int end); On 2011/06/29 09:27:30, Lasse Reichstein wrote:
Slow version of what? Just say what the function does, and only after
that maybe
say which function is supposed to use it.
So it creates a new string, copies prefix[start..end] into the
beginning of it
(and I assume start can be non-zero when prefix is source_) and starts
scanning
the string and adding it after the prefix.
Done. http://codereview.chromium.org/7241023/diff/5001/src/json-parser.h#newcode171 src/json-parser.h:171: // Optimized fast case where we only have ascii characters. On 2011/06/29 09:27:30, Lasse Reichstein wrote:
ASCII is an acronym when used in prose.
Done. http://codereview.chromium.org/7241023/diff/5001/src/json-parser.h#newcode459 src/json-parser.h:459: // Create new seq string On 2011/06/29 09:27:30, Lasse Reichstein wrote:
Move comment to after "if" line, so it only applies if the condition
is true (or
make it apply to the check itself, like below: "Check whether we need
to create
a new string."). Comments should be full sentences (so remember "." at
the end).
No, we don't always follow that in practice :)
Done. http://codereview.chromium.org/7241023/diff/5001/src/json-parser.h#newcode466 src/json-parser.h:466: if (sizeof(char) != sizeof(SinkChar) || On 2011/06/29 09:27:30, Lasse Reichstein wrote:
Does it lint? (Generally, us kCharSize instead of sizeof(char)). For readability, I'd prefer the operands in the opposite order: if (sizeof(SinkChar) != kCharSize || and you should probably do: if (sizeof(SinkChar) == kUC16Size || since you know that there are only those two possibilities.
Done. http://codereview.chromium.org/7241023/diff/5001/src/json-parser.h#newcode467 src/json-parser.h:467: seq_ascii || On 2011/06/29 09:27:30, Lasse Reichstein wrote:
This could use a comment: If the sink can contain UC16 characters, or source_ contains only
ASCII
characters, there's no need to test whether we can store the
character.
Otherwise check whether the UC16 source character can fit in the ASCII
sink. Done. http://codereview.chromium.org/7241023/diff/5001/src/json-parser.h#newcode472 src/json-parser.h:472: // StringType is SeqAsciiString and we just read a non-ascii char. On 2011/06/29 09:27:30, Lasse Reichstein wrote:
non-ASCII.
Done. http://codereview.chromium.org/7241023/diff/5001/src/json-parser.h#newcode510 src/json-parser.h:510: if (sizeof(char) != sizeof(SinkChar) || value <= kMaxAsciiCharCode) { On 2011/06/29 09:27:30, Lasse Reichstein wrote:
sizeof(SinkChar) == kUC16Size
Done. http://codereview.chromium.org/7241023/diff/5001/src/json-parser.h#newcode531 src/json-parser.h:531: template ShrinkStringAtAllocationBoundary<StringType>( On 2011/06/29 09:27:30, Lasse Reichstein wrote:
Do ASSERT that the string is at the allocation boundary. (It should be, there have been no allocations or possible
interruptions since it
was allocated).
That ASSERT is in ShrinkStringAtAllocationBoundary. That should suffice? http://codereview.chromium.org/7241023/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
