Re: Review Request: kjs: Implement JSON.stringify

[Commit Hook]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105057/#review18562
---


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:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105057/
> ---
> 
> (Updated Aug. 13, 2012, 5:36 p.m.)
> 
> 
> Review request for kdelibs.
> 
> 
> Description
> ---
> 
> kjs: Implement JSON.stringify
> 
> patch needs https://git.reviewboard.kde.org/r/105056/ (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: http://git.reviewboard.kde.org/r/105057/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Bernd Buschinski
> 
>

Re: Review Request: kjs: Implement JSON.stringify

[Bernd Buschinski]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105057/
---

(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 https://git.reviewboard.kde.org/r/105056/ (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: http://git.reviewboard.kde.org/r/105057/diff/


Testing
---


Thanks,

Bernd Buschinski

Re: Review Request: kjs: Implement JSON.stringify

[Bernd Buschinski]

> On Aug. 12, 2012, 3:42 p.m., Maks Orlovich wrote:
> > kjs/jsonstringify.cpp, line 102
> > 
> >
> > Resetting m_state, m_rootIsUndefined here might be a good defensive 
> > move (just in case stringify starts getting called twice)

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


- Bernd


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105057/#review17275
---


On July 25, 2012, 2:54 p.m., Bernd Buschinski wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105057/
> ---
> 
> (Updated July 25, 2012, 2:54 p.m.)
> 
> 
> Review request for kdelibs.
> 
> 
> Description
> ---
> 
> kjs: Implement JSON.stringify
> 
> patch needs https://git.reviewboard.kde.org/r/105056/ (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 
>   kjs/ustring.h 49370ef 
> 
> Diff: http://git.reviewboard.kde.org/r/105057/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Bernd Buschinski
> 
>

Re: Review Request: kjs: Implement JSON.stringify

[Maks Orlovich]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105057/#review17275
---

Ship it!


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


kjs/jsonstringify.cpp


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



kjs/jsonstringify.cpp


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



kjs/jsonstringify.cpp


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


Can get an exception here



kjs/jsonstringify.cpp


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:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105057/
> ---
> 
> (Updated July 25, 2012, 2:54 p.m.)
> 
> 
> Review request for kdelibs.
> 
> 
> Description
> ---
> 
> kjs: Implement JSON.stringify
> 
> patch needs https://git.reviewboard.kde.org/r/105056/ (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 
>   kjs/ustring.h 49370ef 
> 
> Diff: http://git.reviewboard.kde.org/r/105057/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Bernd Buschinski
> 
>

Re: Review Request: kjs: Implement JSON.stringify

[Bernd Buschinski]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105057/
---

(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 https://git.reviewboard.kde.org/r/105056/ (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: http://git.reviewboard.kde.org/r/105057/diff/


Testing
---


Thanks,

Bernd Buschinski

Re: Review Request: kjs: Implement JSON.stringify

[Bernd Buschinski]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105057/
---

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


Review request for kdelibs.


Changes
---

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


Description
---

kjs: Implement JSON.stringify

patch needs https://git.reviewboard.kde.org/r/105056/ (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: http://git.reviewboard.kde.org/r/105057/diff/


Testing
---


Thanks,

Bernd Buschinski

Re: Review Request: kjs: Implement JSON.stringify

[Bernd Buschinski]

> On July 4, 2012, 2:30 p.m., Maks Orlovich wrote:
> > kjs/jsonstringify.cpp, line 69
> > 
> >
> > Does this do the right thing if it's shorter than 10?

yes, it does


> On July 4, 2012, 2:30 p.m., Maks Orlovich wrote:
> > kjs/jsonstringify.h, line 65
> > 
> >
> > Don't use std::set, it's sloo. Better use WTF::HashSet if at all 
> > possible.

WTF::HashSet 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


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105057/#review14245
---


On June 1, 2012, 1:30 p.m., Bernd Buschinski wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105057/
> ---
> 
> (Updated June 1, 2012, 1:30 p.m.)
> 
> 
> Review request for kdelibs.
> 
> 
> Description
> ---
> 
> kjs: Implement JSON.stringify
> 
> patch needs https://git.reviewboard.kde.org/r/105056/ (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: http://git.reviewboard.kde.org/r/105057/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Bernd Buschinski
> 
>

Re: Review Request: kjs: Implement JSON.stringify

[Maks Orlovich]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105057/#review15376
---



kjs/jsonstringify.cpp


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


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


Just '\\'.




kjs/jsonstringify.cpp


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



kjs/jsonstringify.cpp


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


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




kjs/jsonstringify.cpp


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:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105057/
> ---
> 
> (Updated June 1, 2012, 1:30 p.m.)
> 
> 
> Review request for kdelibs.
> 
> 
> Description
> ---
> 
> kjs: Implement JSON.stringify
> 
> patch needs https://git.reviewboard.kde.org/r/105056/ (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: http://git.reviewboard.kde.org/r/105057/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Bernd Buschinski
> 
>

Re: Review Request: kjs: Implement JSON.stringify

[Maks Orlovich]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105057/#review14245
---



kjs/jsonstringify.h


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



kjs/jsonstringify.cpp


Remember, toString can throw exceptions :(




kjs/jsonstringify.cpp


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:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105057/
> ---
> 
> (Updated June 1, 2012, 1:30 p.m.)
> 
> 
> Review request for kdelibs.
> 
> 
> Description
> ---
> 
> kjs: Implement JSON.stringify
> 
> patch needs https://git.reviewboard.kde.org/r/105056/ (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: http://git.reviewboard.kde.org/r/105057/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Bernd Buschinski
> 
>

Re: Review Request: kjs: Implement JSON.stringify

[Bernd Buschinski]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105057/
---

(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 https://git.reviewboard.kde.org/r/105056/ (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: http://git.reviewboard.kde.org/r/105057/diff/


Testing
---


Thanks,

Bernd Buschinski

Re: Review Request: kjs: Implement JSON.stringify

[Bernd Buschinski]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105057/
---

(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 https://git.reviewboard.kde.org/r/105056/ (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: http://git.reviewboard.kde.org/r/105057/diff/


Testing
---


Thanks,

Bernd Buschinski

Re: Review Request: kjs: Implement JSON.stringify

[Rolf Eike Beer]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105057/#review14178
---


You forgot to change kjs/tests/ecmatest_broken_*


kjs/json_object.cpp


Duplicate newline


- Rolf Eike Beer


On May 25, 2012, 8:19 p.m., Bernd Buschinski wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105057/
> ---
> 
> (Updated May 25, 2012, 8:19 p.m.)
> 
> 
> Review request for kdelibs.
> 
> 
> Description
> ---
> 
> kjs: Implement JSON.stringify
> 
> patch needs https://git.reviewboard.kde.org/r/105056/ (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: http://git.reviewboard.kde.org/r/105057/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Bernd Buschinski
> 
>

Review Request: kjs: Implement JSON.stringify

[Bernd Buschinski]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105057/
---

Review request for kdelibs.


Description
---

kjs: Implement JSON.stringify

patch needs https://git.reviewboard.kde.org/r/105056/ (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: http://git.reviewboard.kde.org/r/105057/diff/


Testing
---


Thanks,

Bernd Buschinski