Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

more c++ #7

Open
azadkuh opened this issue Jun 16, 2014 · 5 comments
Open

more c++ #7

azadkuh opened this issue Jun 16, 2014 · 5 comments

Comments

@azadkuh
Copy link

azadkuh commented Jun 16, 2014

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

@ChrisJefferson
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Author

azadkuh commented Jun 16, 2014

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
Copy link
Contributor

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
Copy link

duckie commented Jul 20, 2015

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants