From kde-core-devel Sun Jun 10 18:57:48 2012 From: "Maks Orlovich" Date: Sun, 10 Jun 2012 18:57:48 +0000 To: kde-core-devel Subject: Re: Review Request: kjs: Implement JSON.parse Message-Id: <20120610185748.7789.21836 () vidsolbach ! de> X-MARC-Message: https://marc.info/?l=kde-core-devel&m=133935488217294 MIME-Version: 1 Content-Type: multipart/mixed; boundary="--===============3705380274313717730==" --===============3705380274313717730== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105056/#review14590 ----------------------------------------------------------- kjs/json_object.cpp What namespace do the symbols for this end up in? = kjs/json_object.cpp What happens if the call threw an exception? = kjs/json_object.cpp I think it's true, but needs one more proviso: the check on line 89. kjs/json_object.cpp I think if you don't put in a space between * and /* some Windows compi= ler will whine. (Warnings only, I think, but someone was bothered by it enough in the p= ast to ask us to change a bunch) kjs/json_object.cpp What happens if toObject throws? (I think it can.. but I might be wrong= ). = kjs/jsonlexer.h explicit kjs/jsonlexer.h explicit kjs/jsonlexer.h What's the return value on failure? kjs/jsonlexer.cpp static = kjs/jsonlexer.cpp static kjs/jsonlexer.cpp static, comment pareameters? = kjs/jsonlexer.cpp I don't think this is needed anymore? (Nor the similar one in arrays) kjs/jsonlexer.cpp This looks like it will run out of stack space pretty quickly. I think = it would be better to instead of firstArrayRun to have something more like = what you have for objects. (Or just use propAdded in the same way) = kjs/jsonlexer.cpp So JSON doesn't have 'strings'? = kjs/jsonlexer.cpp Where did the -? go? = kjs/jsonlexer.cpp Comment method? - Maks Orlovich On May 28, 2012, 7:52 p.m., Bernd Buschinski wrote: > = > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/105056/ > ----------------------------------------------------------- > = > (Updated May 28, 2012, 7:52 p.m.) > = > = > Review request for kdelibs. > = > = > Description > ------- > = > kjs: Implement JSON.parse > = > = > Diffs > ----- > = > kjs/CMakeLists.txt 1188064 = > kjs/interpreter.cpp cf1acf1 = > kjs/json_object.h PRE-CREATION = > kjs/json_object.cpp PRE-CREATION = > kjs/jsonlexer.h PRE-CREATION = > kjs/jsonlexer.cpp PRE-CREATION = > kjs/libkjs.map e9f679f = > = > Diff: http://git.reviewboard.kde.org/r/105056/diff/ > = > = > Testing > ------- > = > = > Thanks, > = > Bernd Buschinski > = > --===============3705380274313717730== Content-Type: text/html; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable
This is an automatically generated e-mail. To reply, visit: http://git.revie= wboard.kde.org/r/105056/

= =
kjs/json_object.cpp (Diff revision 3) 

   
const ClassInfo JSONObjectImp::info =3D { "JSON", 0, &=
;jsonTable, 0 };


  

 

  

    
    

    37
    
@begin jsonTable 1

  


 





What namespace do the symbols for this end up in?


















 =

 =




 

 
  

   
    kjs/json_object.cpp
    

     (Diff revision 3)

    
   
  

 

 
  


   

   
const ClassInfo JSONObjectImp::info =3D { "JSON", 0, &=
;jsonTable, 0 };


  

 

  

    
    

    88
    
                JSValue* ret =3D func->call(exec, obj, args);
<=
/td>
  


 





What happens if the call threw an exception?


















 =

 =




 

 
  

   
    kjs/json_object.cpp
    

     (Diff revision 3)

    
   
  

 

 
  


   

   
const ClassInfo JSONObjectImp::info =3D { "JSON", 0, &=
;jsonTable, 0 };


  

 

  

    
    

    107
    
            // and we only =
have json data here

  


 





I think it's true, but needs one more proviso: the check on line=
 89.

















 =

 =




 

 
  

   
    kjs/json_object.cpp
    

     (Diff revision 3)

    
   
  

 

 
  


   

   
const ClassInfo JSONObjectImp::info =3D { "JSON", 0, &=
;jsonTable, 0 };


  

 

  

    
    

    113
    
JSValue *JSONFuncImp::callAsFunction(ExecState *exe=
c, JSObject */*thisObj<=
/span>*/, const List &args)

  


 





I think if you don't put in a space between * and /* some Window=
s compiler will whine.
(Warnings only, I think, but someone was bothered by it enough in the past =
to ask us to change a bunch)

















 =

 =




 

 
  

   
    kjs/json_object.cpp
    

     (Diff revision 3)

    
   
  

 

 
  


   

   
const ClassInfo JSONObjectImp::info =3D { "JSON", 0, &=
;jsonTable, 0 };


  

 

  

    
    

    139
    
                JSValue* ret =3D function->=
call(ex=
ec, val->toObject(exec), args);

  


 





What happens if toObject throws? (I think it can.. but I might be wr=
ong).


















 =

 =




 

 
  

   
    kjs/jsonlexer.h
    

     (Diff revision 3)

    
   
  

 

 
  


   

   
public:


  

 

  

    
    

    64
    
    JSONLexer(const UString<=
/span>& code);

  


 





explicit

















 =

 =




 

 
  

   
    kjs/jsonlexer.h
    

     (Diff revision 3)

    
   
  

 

 
  


   

   
public:


  

 

  

    
    

    88
    
    JSONParser(const UString=
& code);

  


 





explicit

















 =

 =




 

 
  

   
    kjs/jsonlexer.h
    

     (Diff revision 3)

    
   
  

 

 
  


   

   
public:


  

 

  

    
    

    90
    
    JSValue* tryParse(ExecState* exec);

  


 





What's the return value on failure?

















 =

 =




 

 
  

   
    kjs/jsonlexer.cpp
    

     (Diff revision 3)

    
   
  

 

 
  


   

   
static inline bool isJSONWhiteSpace(const UChar& c)


  

 

  

    
    

    66
    
inline UString tokenToString(TokenType type=
) {

  


 





static


















 =

 =




 

 
  

   
    kjs/jsonlexer.cpp
    

     (Diff revision 3)

    
   
  

 

 
  


   

   
static inline bool isJSONWhiteSpace(const UChar& c)


  

 

  

    
    

    89
    
inline UString parserStateToString(ParserState sta=
te) {

  


 





static

















 =

 =




 

 
  

   
    kjs/jsonlexer.cpp
    

     (Diff revision 3)

    
   
  

 

 
  


   

   
static inline bool isJSONWhiteSpace(const UChar& c)


  

 

  

    
    

    121
    
inline bool addArrayItem=
(ExecState* exec, std::stack<JSValue*>* arrayStack, JSValue* value, JSObject* object)

  


 





static, comment pareameters?


















 =

 =




 

 
  

   
    kjs/jsonlexer.cpp
    

     (Diff revision 3)

    
   
  

 

 
  


   

   
static inline bool isJSONWhiteSpace(const UChar& c)


  

 

  

    
    

    193
    
                // this is =
needed for empty objects "[{}]"

  


 





I don't think this is needed anymore?
(Nor the similar one in arrays)

















 =

 =




 

 
  

   
    kjs/jsonlexer.cpp
    

     (Diff revision 3)

    
   
  

 

 
  


   

   
static inline bool isJSONWhiteSpace(const UChar& c)


  

 

  

    
    

    292
    
                        if (!addArrayItem(exec=
, &=
arrayObjectStack, parse(exec, JSONValue), <=
span class=3D"n">object))

  


 





This looks like it will run out of stack space pretty quickly. I thi=
nk it would be better to instead of firstArrayRun to have something more li=
ke what you have for objects. (Or just use propAdded in the same way)


















 =

 =




 

 
  

   
    kjs/jsonlexer.cpp
    

     (Diff revision 3)

    
   
  

 

 
  


   

   
static inline bool isJSONWhiteSpace(const UChar& c)


  

 

  

    
    

    359
    
    while (!(m_code[m_pos] =3D=3D '"')) {

  


 





So JSON doesn't have 'strings'?


















 =

 =




 

 
  

   
    kjs/jsonlexer.cpp
    

     (Diff revision 3)

    
   
  

 

 
  


   

   
static inline bool isJSONWhiteSpace(const UChar& c)


  

 

  

    
    

    398
    
    // -?(0 | [1-9][0-9]*) =
('.' [0-9]+)? ([eE][+-]? [0-9]+)?

  


 





Where did the -? go?


















 =

 =




 

 
  

   
    kjs/jsonlexer.cpp
    

     (Diff revision 3)

    
   
  

 

 
  


   

   
static inline bool isJSONWhiteSpace(const UChar& c)


  

 

  

    
    

    510
    
static inline bool isStrin=
gSequence(int pos, const=
 UString& code, const=
 UString& word)

  


 





Comment method?









- Maks






On May 28th, 2012, 7:52 p.m., Bernd Buschinski wrote:









 

  


Review request for kdelibs.


By Bernd Buschinski.




Updated May 28, 2012, 7:52 p.m.








Descripti=
on 

 

 

  
   
kjs: Implement JSON.parse

  		
 









Diffs=
 


    

 
  • kjs/CMakeLists.txt (1188064)
    • 

 
  • kjs/interpreter.cpp (cf1acf1)
    • 

 
  • kjs/json_object.h (PRE-CREATION)

 
  • kjs/json_object.cpp (PRE-CREATION)<=
/li>

 
  • kjs/jsonlexer.h (PRE-CREATION)
    • 

 
  • kjs/jsonlexer.cpp (PRE-CREATION)

 
  • kjs/libkjs.map (e9f679f)
    • 





View Diff





  		
 











  

 



--===============3705380274313717730==--