vivkin/gason

more c++

Opened this issue · 5 comments

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

  • using gason namespace
  • add a few method for finding children and array items.
  • overloading operators for validity and type (tag) checking and ...
  • adding a simple JSon builder class.

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

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?

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.

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.

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.

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