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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20120704/f49ec620/attachment.htm>