vivkin / gason

Lightweight and fast JSON parser for C++
MIT License
340 stars 50 forks source link

more c++ #7

Open azadkuh opened 10 years ago

azadkuh commented 10 years ago

dear vivkin;

I enjoyed your parser a lot esp for its low memory footprint.

To mold the api into more c++ (i don't mean std::) and give the end-user even more simple API I've made some small changes in gason to form gason++

because these changes breaks your API (just using namespace gason; should be added to legacy codes) I prefer not to fork and submit an immediate pull request.

but if you find these changes useful please merge them into gason: https://github.com/azadkuh/gason++

ChrisJefferson commented 10 years ago

There is far too many things here (in my opinion) dumped into one patch, and you have changed the formatting of every line of code. It doesn't seem possible to unpick things from this!

Some minor comments:

1) You seem to have removed the C++11 from various places, that seems bad to me.

2) You have switched some loops to checking 'isValid()' on iterators as the end-of-loop condition. That is very un-C++ like.

3) Your simple JSon builder class seems to require I give a fixed sized buffer, and then not check the size of the buffer for overflow. That seems horribly unsafe. Why not use a ostringstream, which is built into C++ and safer?

ChrisJefferson commented 10 years ago

Other issues: at() / operator[] return a null if the object being dereferences is not an array, or for out-of-bounds. It seems more 'gason' to assert if we try to dereference something which is not an array.

azadkuh commented 10 years ago

chris;

1) I've not changed every line, just cleaned the white-spaces (tab to 4 spaces). it's not a deal breaker, only a personal taste. the new API is backward compatible with original gason (except for namespace).

2) you are right. I personally love c++11 a lot, but I'm doing some tests on an ARM9 POS device with an old toolchain (gcc 4.2). c++11 apart, every thing seems to be Ok right now with gason++.

3) I'm considering to rename isValid() to hasNext(); it's very common for list iterators. (Qt, ...)

4) The StringBuilder (JSonbuilder' base class) checks for remaining empty spaces and rejects adding extra stuff to internal buffer. The output JSon is not valid but the there is no buffer overflow. you can try it. (std::ostringstream is a no go on memory limited devices, in embedded world alloc/free or new/delete are risky because there is no MMU and memory fragmentation causes serious problem).

5) I will fix at() issue. thank you.

ChrisJefferson commented 10 years ago

The main problem with the whitespace changes is that is makes it very difficult to see what changes you have made, as when I run the code through a differ it says everything has changed! I'll try looking at disabling white-space only changes.

duckie commented 9 years ago

If you happen to be a vim user, use the diffopt option of vimdiff to hide whitespace only changes.