DevHeads.net

Review Request: kjs: Implement JSON.stringify

Review request for kdelibs.

Description
kjs: Implement JSON.stringify

patch needs <a href="https://git.reviewboard.kde.org/r/105056/" title="https://git.reviewboard.kde.org/r/105056/">https://git.reviewboard.kde.org/r/105056/</a> (JSON.parse)

Diffs
kjs/CMakeLists.txt 1188064
kjs/CommonIdentifiers.h 8ee40e8
kjs/json_object.h PRE-CREATION
kjs/json_object.cpp PRE-CREATION
kjs/jsonstringify.h PRE-CREATION
kjs/jsonstringify.cpp PRE-CREATION

Diff: <a href="http://git.reviewboard.kde.org/r/105057/diff/" title="http://git.reviewboard.kde.org/r/105057/diff/">http://git.reviewboard.kde.org/r/105057/diff/</a>

Testing

Thanks,

Bernd Buschinski

Comments

Re: Review Request: kjs: Implement JSON.stringify

By Bernd Buschinski at 05/27/2012 - 11:25

(Updated May 27, 2012, 3:25 p.m.)

Review request for kdelibs.

Changes
- fixed double newline
- fixed "functions" could not be replaced, through replace function

now it passes all ecma JSON.stringify tests

this means removing all 15.12.3* test from broken file (not yet included in diff)

Description
kjs: Implement JSON.stringify

patch needs <a href="https://git.reviewboard.kde.org/r/105056/" title="https://git.reviewboard.kde.org/r/105056/">https://git.reviewboard.kde.org/r/105056/</a> (JSON.parse)

Diffs (updated)
kjs/CMakeLists.txt 1188064
kjs/CommonIdentifiers.h 8ee40e8
kjs/json_object.h PRE-CREATION
kjs/json_object.cpp PRE-CREATION
kjs/jsonstringify.h PRE-CREATION
kjs/jsonstringify.cpp PRE-CREATION

Diff: <a href="http://git.reviewboard.kde.org/r/105057/diff/" title="http://git.reviewboard.kde.org/r/105057/diff/">http://git.reviewboard.kde.org/r/105057/diff/</a>

Testing

Thanks,

Bernd Buschinski

Re: Review Request: kjs: Implement JSON.stringify

By Bernd Buschinski at 06/01/2012 - 09:30

(Updated June 1, 2012, 1:30 p.m.)

Review request for kdelibs.

Changes
- properly remember holder
- fixes crash for js code like

var b = 42;
function white(a,b,c) { return 10; }
var o = JSON.stringify(b, white);

Description
kjs: Implement JSON.stringify

patch needs <a href="https://git.reviewboard.kde.org/r/105056/" title="https://git.reviewboard.kde.org/r/105056/">https://git.reviewboard.kde.org/r/105056/</a> (JSON.parse)

Diffs (updated)
kjs/CMakeLists.txt 1188064
kjs/CommonIdentifiers.h 8ee40e8
kjs/json_object.h PRE-CREATION
kjs/json_object.cpp PRE-CREATION
kjs/jsonstringify.h PRE-CREATION
kjs/jsonstringify.cpp PRE-CREATION

Diff: <a href="http://git.reviewboard.kde.org/r/105057/diff/" title="http://git.reviewboard.kde.org/r/105057/diff/">http://git.reviewboard.kde.org/r/105057/diff/</a>

Testing

Thanks,

Bernd Buschinski

Re: Review Request: kjs: Implement JSON.stringify

By Bernd Buschinski at 07/04/2012 - 18:27

(Updated July 4, 2012, 10:27 p.m.)

Review request for kdelibs.

Changes
fixed, beside from the std::set<UString> -> WTF::HashSet<UString> change...
I currently can't get it compile, it just drives me mad...

Description
kjs: Implement JSON.stringify

patch needs <a href="https://git.reviewboard.kde.org/r/105056/" title="https://git.reviewboard.kde.org/r/105056/">https://git.reviewboard.kde.org/r/105056/</a> (JSON.parse)

Diffs (updated)
kjs/CMakeLists.txt 1188064
kjs/CommonIdentifiers.h 8ee40e8
kjs/json_object.h PRE-CREATION
kjs/json_object.cpp PRE-CREATION
kjs/jsonstringify.h PRE-CREATION
kjs/jsonstringify.cpp PRE-CREATION

Diff: <a href="http://git.reviewboard.kde.org/r/105057/diff/" title="http://git.reviewboard.kde.org/r/105057/diff/">http://git.reviewboard.kde.org/r/105057/diff/</a>

Testing

Thanks,

Bernd Buschinski

Re: Review Request: kjs: Implement JSON.stringify

By Bernd Buschinski at 07/25/2012 - 10:54

(Updated July 25, 2012, 2:54 p.m.)

Review request for kdelibs.

Changes
now with WTF::HashSet instead of std::set

Description
kjs: Implement JSON.stringify

patch needs <a href="https://git.reviewboard.kde.org/r/105056/" title="https://git.reviewboard.kde.org/r/105056/">https://git.reviewboard.kde.org/r/105056/</a> (JSON.parse)

Diffs (updated)
kjs/CMakeLists.txt 1188064
kjs/CommonIdentifiers.h 8ee40e8
kjs/json_object.h PRE-CREATION
kjs/json_object.cpp PRE-CREATION
kjs/jsonstringify.h PRE-CREATION
kjs/jsonstringify.cpp PRE-CREATION
kjs/ustring.h 49370ef

Diff: <a href="http://git.reviewboard.kde.org/r/105057/diff/" title="http://git.reviewboard.kde.org/r/105057/diff/">http://git.reviewboard.kde.org/r/105057/diff/</a>

Testing

Thanks,

Bernd Buschinski

Re: Review Request: kjs: Implement JSON.stringify

By Bernd Buschinski at 08/13/2012 - 13:36

(Updated Aug. 13, 2012, 5:36 p.m.)

Review request for kdelibs.

Changes
now StackObjectLimit 1500, firefox-14 seems to have something like ~1480,
rekonq&opera seem to have no limit and eat all my memory in the endless case

Description
kjs: Implement JSON.stringify

patch needs <a href="https://git.reviewboard.kde.org/r/105056/" title="https://git.reviewboard.kde.org/r/105056/">https://git.reviewboard.kde.org/r/105056/</a> (JSON.parse)

Diffs (updated)
kjs/CMakeLists.txt 1188064
kjs/CommonIdentifiers.h 8ee40e8
kjs/json_object.h PRE-CREATION
kjs/json_object.cpp PRE-CREATION
kjs/jsonstringify.h PRE-CREATION
kjs/jsonstringify.cpp PRE-CREATION

Diff: <a href="http://git.reviewboard.kde.org/r/105057/diff/" title="http://git.reviewboard.kde.org/r/105057/diff/">http://git.reviewboard.kde.org/r/105057/diff/</a>

Testing

Thanks,

Bernd Buschinski

Re: Review Request: kjs: Implement JSON.stringify

By Commit Hook at 09/05/2012 - 14:37

This review has been submitted with commit 930a86b4ad07fed47179067711f97dd0edc1d805 by Bernd Buschinski to branch KDE/4.9.

- Commit Hook

On Aug. 13, 2012, 5:36 p.m., Bernd Buschinski wrote:

Re: Review Request: kjs: Implement JSON.stringify

By Maks Orlovich at 08/12/2012 - 11:42

Ship it!

Almost there; will be OK iff everything below is fixed.

kjs/jsonstringify.cpp
<http://git.reviewboard.kde.org/r/105057/#comment13452>

Does this do the right thing with NaN and the like?

kjs/jsonstringify.cpp
<http://git.reviewboard.kde.org/r/105057/#comment13453>

Resetting m_state, m_rootIsUndefined here might be a good defensive move (just in case stringify starts getting called twice)

kjs/jsonstringify.cpp
<http://git.reviewboard.kde.org/r/105057/#comment13454>

This looks like we could end up looping indefinitely here and crashing with out of stack depth (if they keep returning new objects which have .toJSON set0. I think some checking for m_objectStack.size() may be in order. Probably a good idea in case someone tries to serialize something really deep, too.

kjs/jsonstringify.cpp
<http://git.reviewboard.kde.org/r/105057/#comment13455>

Can get an exception here

kjs/jsonstringify.cpp
<http://git.reviewboard.kde.org/r/105057/#comment13456>

You don't want to be keying by UString here, but rather than Identifier, since they guarantee reference equality of equal strings, unlike UString.

CommonIdentifiers.h has the traits for them.

- Maks Orlovich

On July 25, 2012, 2:54 p.m., Bernd Buschinski wrote:

Re: Review Request: kjs: Implement JSON.stringify

By Bernd Buschinski at 08/13/2012 - 13:36

no, I can't reset m_state here, it might already be Failed* because of the values given in the ctor

- Bernd

On July 25, 2012, 2:54 p.m., Bernd Buschinski wrote:

Re: Review Request: kjs: Implement JSON.stringify

By Maks Orlovich at 07/04/2012 - 10:53

kjs/jsonstringify.cpp
<http://git.reviewboard.kde.org/r/105057/#comment11977>

You shouldn't use all property names here, but only ones that are array indeces - -- see 15.12.3, step 4.b.ii:

"For each value v of a property of replacer that has an array index property name. The
properties are enumerated in the ascending array index order of their names."

See also toArrayIndex in our code (and beware 2^32-1, which isn't an array index index despite being a uint32)

kjs/jsonstringify.cpp
<http://git.reviewboard.kde.org/r/105057/#comment11978>

Double-check behavior on NaN/-inf/+inf?

Also, actually toInteger and above toString can technically throw, too, since things like String.prototype.toString are replaceable.

kjs/jsonstringify.cpp
<http://git.reviewboard.kde.org/r/105057/#comment11979>

Just '\\'.

kjs/jsonstringify.cpp
<http://git.reviewboard.kde.org/r/105057/#comment11980>

I can't immediately see why this can't be null.

kjs/jsonstringify.cpp
<http://git.reviewboard.kde.org/r/105057/#comment11981>

This looks dubious, as toString and toNumber can be independently customized. I think you want to use UString::from(double) on the value instead.

kjs/jsonstringify.cpp
<http://git.reviewboard.kde.org/r/105057/#comment11982>

hmm, should it be this or ->implementsCall()?

kjs/jsonstringify.cpp
<http://git.reviewboard.kde.org/r/105057/#comment11984>

Again, I am pretty sure this is supposed to only list array properties, and not symbolic ones, which you would get via getPropertyNames.

- Maks Orlovich

On June 1, 2012, 1:30 p.m., Bernd Buschinski wrote:

Re: Review Request: kjs: Implement JSON.stringify

By Maks Orlovich at 07/04/2012 - 10:30

kjs/jsonstringify.h
<http://git.reviewboard.kde.org/r/105057/#comment11267>

Don't use std::set, it's sloowwww. Better use WTF::HashSet if at all possible.

kjs/jsonstringify.cpp
<http://git.reviewboard.kde.org/r/105057/#comment11268>

Remember, toString can throw exceptions :(

kjs/jsonstringify.cpp
<http://git.reviewboard.kde.org/r/105057/#comment11269>

Does this do the right thing if it's shorter than 10?

- Maks Orlovich

On June 1, 2012, 1:30 p.m., Bernd Buschinski wrote:

Re: Review Request: kjs: Implement JSON.stringify

By Bernd Buschinski at 07/04/2012 - 18:27

yes, it does

WTF::HashSet<UString> just drives me mad, it won't compile easily that way.
I only found UString::Rep* HashSet useage in kjs, but just storing the rep in this case does not work...

- Bernd

On June 1, 2012, 1:30 p.m., Bernd Buschinski wrote:

Re: Review Request: kjs: Implement JSON.stringify

By Rolf Eike Beer at 05/26/2012 - 11:10

You forgot to change kjs/tests/ecmatest_broken_*

kjs/json_object.cpp
<http://git.reviewboard.kde.org/r/105057/#comment11211>

Duplicate newline

- Rolf Eike Beer

On May 25, 2012, 8:19 p.m., Bernd Buschinski wrote: