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

Iterator pointer semantics fix #18

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

duckie
Copy link

@duckie duckie commented Jul 20, 2015

Hello good sir

While writing a wrapper around gason for my company, I ran into a small problem. Here is a fix proposal.

API breakage here, but original API is erroneous.

The JsonIterator::operator* in gason does not implement correctly the pointer semantics. An iterator should be used in those ways (assuming v is a value containing an object).

for(auto it = begin(v); it != end(v); ++it)
  cout << it->key << endl;
for(auto node : v)
  cout << node.key << endl;

But in gason is used like this:

for(auto node : v)
  cout << node->key << endl;

This is not consistent with common behaviors of C++ iterators. This patch fixes it.

@ChrisJefferson
Copy link
Contributor

While I actually agree with this patch in principle, I would hope it would wait until a major release, as it's going to break every piece of code using gason!

@duckie
Copy link
Author

duckie commented Jul 21, 2015

Yes it does. There is no problem into delaying it for a major release. Do you plan one ? Do you want me to request the pull into a release candidate branch ?

@ChrisJefferson
Copy link
Contributor

Just to say, I'm just a random guy who read and commented on your patch, not @vivkin :)

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

Successfully merging this pull request may close these issues.

None yet

2 participants