I think the pop() is fine, because you need to do a pop() anyway in the non-empty cases. LGTM
On Sun, Dec 5, 2010 at 4:09 PM, <[email protected]> wrote: > STV! > > > 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() != ",") { > 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), ":"); > 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() != ",") { > 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")); > I wonder how $String compares speedwise to ("" + value). > > http://codereview.chromium.org/5567005/diff/1/src/json.js#newcode270 > src/json.js:270: } else { // Regular non-wrapped object > There should be 2 spaces before // > > > http://codereview.chromium.org/5567005/ > -- William Hesse Software Engineer [email protected] Google Denmark ApS Frederiksborggade 20B, 1 sal 1360 København K Denmark CVR nr. 28 86 69 84 If you received this communication by mistake, please don't forward it to anyone else (it may contain confidential or privileged information), please erase all copies of it, including all attachments, and please let the sender know it went to the wrong person. Thanks. -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
