----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105056/#review14235 -----------------------------------------------------------
kjs/json_object.cpp <http://git.reviewboard.kde.org/r/105056/#comment11249> Comment needs fixing. kjs/json_object.cpp <http://git.reviewboard.kde.org/r/105056/#comment11250> Please try to be consistent with * and & placement at least at the level of a single line. kjs/json_object.cpp <http://git.reviewboard.kde.org/r/105056/#comment11252> static; otherwise you're polluting global namespace. kjs/json_object.cpp <http://git.reviewboard.kde.org/r/105056/#comment11257> The spec seems to call for special handling of arrays, are you sure you can get away with not doing it? (You probably can given it's parse output, but it could use a comment). kjs/json_object.cpp <http://git.reviewboard.kde.org/r/105056/#comment11256> Are you sure about the 'IncludeDontEnum' bit? The Walk definition seems to require going only through enumerable ones. kjs/json_object.cpp <http://git.reviewboard.kde.org/r/105056/#comment11258> I think you end up not calling func on the very top-most object. Also, what's the semantics here wrt to exceptions? I think you may be doing too much if func throws. kjs/json_object.cpp <http://git.reviewboard.kde.org/r/105056/#comment11253> Again, the */& placement is all over the place. kjs/json_object.cpp <http://git.reviewboard.kde.org/r/105056/#comment11254> Are you sure this is correct? The JS-ey thing would be to try to parse 'undefined'. kjs/json_object.cpp <http://git.reviewboard.kde.org/r/105056/#comment11255> The ->toString could have thrown an exception... kjs/jsonlexer.h <http://git.reviewboard.kde.org/r/105056/#comment11259> Comment that it may return NULL (and not just jsUndefined). kjs/jsonlexer.cpp <http://git.reviewboard.kde.org/r/105056/#comment11264> Why not use the numeric variant instead? kjs/jsonlexer.cpp <http://git.reviewboard.kde.org/r/105056/#comment11260> define 'later' please. kjs/jsonlexer.cpp <http://git.reviewboard.kde.org/r/105056/#comment11261> Shouldn't you at return a parse error here if propertyName is empty? Also note the difference between "": 42 and :42 -- not sure if former is legit. kjs/jsonlexer.cpp <http://git.reviewboard.kde.org/r/105056/#comment11262> again, "later"? Sounds like you've already implemented it. kjs/jsonlexer.cpp <http://git.reviewboard.kde.org/r/105056/#comment11263> Err, does it mean it will accept [1 2 3 4] as an array of 4 numbers? That doesn't look right. For that matter even [1,2,] doesn't look right in JSON. kjs/jsonlexer.cpp <http://git.reviewboard.kde.org/r/105056/#comment11265> It's generally suggested to use pointers for out params. kjs/jsonlexer.cpp <http://git.reviewboard.kde.org/r/105056/#comment11266> And what if all valid characters in the buffer are whitespace? You'll run past the end of it. Also, this code isn't robust if ::next() is called at end-of-file more than once. I would suggest something like: while(true) { if (m_pos >= m_code.size()) { m_type = TokEnd; return m_type; } if (!isJSONWhiteSpace(m_code[m_pos])) { break; } ++m_pos; } - Maks Orlovich On May 27, 2012, 3:30 p.m., Bernd Buschinski wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/105056/ > ----------------------------------------------------------- > > (Updated May 27, 2012, 3:30 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 > >
