http://codereview.chromium.org/5567005/diff/1/src/json.js
File src/json.js (right):
http://codereview.chromium.org/5567005/diff/1/src/json.js#newcode213
src/json.js:213: if (builder.pop() != ",") {
I added a comment.
On 2010/12/05 15:09:06, Erik Corry wrote:
This seems a bit indirect. I think it would be clearer to just say if
(len ==
0). Is this version faster?
http://codereview.chromium.org/5567005/diff/1/src/json.js#newcode229
src/json.js:229: builder.push(%QuoteJSONString(p), ":");
I experimented a bit. Will submit another change.
On 2010/12/05 15:09:06, Erik Corry wrote:
If you didn't already try it I think it would be interesting to
experiment with
having QuoteJSONString not add the quotes. We should be faster at
adding the
quotes now and it would open up the possibility of just returning the
string
unchanged which should be the common case. We can save that for
another change
though.
http://codereview.chromium.org/5567005/diff/1/src/json.js#newcode241
src/json.js:241: if (builder.pop() != ",") {
I added a comment
On 2010/12/05 15:09:06, Erik Corry wrote:
Again, I find this convoluted. A boolean variable that gets set if
the for loop
is non-empty would be clearer.
http://codereview.chromium.org/5567005/diff/1/src/json.js#newcode258
src/json.js:258: builder.push(($isFinite(value) ? $String(value) :
"null"));
On 2010/12/06 08:48:45, Lasse Reichstein wrote:
Use %_NumberToString instead of the generic $String. You know it's a
number. (Or
use NonStringToString, but it will just test for being a number and
call
%_NumberToString)
Done.
http://codereview.chromium.org/5567005/diff/1/src/json.js#newcode258
src/json.js:258: builder.push(($isFinite(value) ? $String(value) :
"null"));
On 2010/12/05 15:09:06, Erik Corry wrote:
I wonder how $String compares speedwise to ("" + value).
Done.
http://codereview.chromium.org/5567005/diff/1/src/json.js#newcode264
src/json.js:264: value = $Number(value);
On 2010/12/06 08:48:45, Lasse Reichstein wrote:
use %_ValueOf to extract the number.
Done.
http://codereview.chromium.org/5567005/diff/1/src/json.js#newcode265
src/json.js:265: builder.push(($isFinite(value) ? $String(value) :
"null"));
On 2010/12/06 08:48:45, Lasse Reichstein wrote:
%_NumberToString here too.
Done.
http://codereview.chromium.org/5567005/diff/1/src/json.js#newcode270
src/json.js:270: } else { // Regular non-wrapped object
On 2010/12/05 15:09:06, Erik Corry wrote:
There should be 2 spaces before //
Done.
http://codereview.chromium.org/5567005/diff/1/src/json.js#newcode270
src/json.js:270: } else { // Regular non-wrapped object
On 2010/12/06 08:48:45, Lasse Reichstein wrote:
I would put the comment on the next line instead.
Done.
http://codereview.chromium.org/5567005/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
