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

Adding first and second properties to iteration_proxy_internal #578

Merged
merged 1 commit into from
May 10, 2017

Conversation

Type1J
Copy link
Contributor

@Type1J Type1J commented May 10, 2017

This PR fixes #350 by adding property-like members to the iterator_proxy_internal class.

The fix proposed in that issue looks as if you pay for it even if you don't use .first or .second (at least the .first shows a copy of a string each time the iterator is incremented). That cost shouldn't have to be payed, but we don't have to sacrifice the interface that we want.

This solution provides the .first and .second interface as outlined in the issue, but shouldn't cost anything, and is likely inlined out of existence after compilation.

@Type1J
Copy link
Contributor Author

Type1J commented May 10, 2017

Here's the test that I used:

#include <json.hpp>
#include <iostream>

using namespace std;
using namespace nlohmann;

int main()
{
  json j = {{"A", 1}, {"B", 2}};

  for (auto i : json::iterator_wrapper(j))
  {
    auto x = i.first; // x = "A", x = "B"
    auto y = i.second; // y = 1, y = 2

    cout << x << ", " << y << endl;
  }
}

Here's the output:

"A", 1
"B", 2

@nlohmann
Copy link
Owner

This is a nice solution! Thanks!

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0004%) to 99.722% when pulling 6a656ed on Type1J:develop_feature_first_second into 2afbd33 on nlohmann:develop.

@nlohmann nlohmann self-assigned this May 10, 2017
@nlohmann nlohmann added this to the Release 3.0.0 milestone May 10, 2017
@nlohmann nlohmann merged commit 5beea35 into nlohmann:develop May 10, 2017
@Type1J Type1J deleted the develop_feature_first_second branch May 10, 2017 15:36
@nlohmann
Copy link
Owner

Unfortunately, the code does not work for me. If you look into the test suite in tests/src/unit-iterator_wrapper.cpp, replacing calls to key() withfirst and value() with second yields compilation errors.

It would be great if you could have a look at this.

@Type1J
Copy link
Contributor Author

Type1J commented May 10, 2017

I'm using MSVC 2017. What compiler failed for you?

@nlohmann
Copy link
Owner

clang version 5.0.0 (trunk 292523) (llvm/trunk 292718) - in any case, it would be great to have unit tests - I should not have merged the code without.

@Type1J
Copy link
Contributor Author

Type1J commented May 10, 2017

I'll look into it, and I'll see about adding the unit tests as well.

@Type1J Type1J restored the develop_feature_first_second branch May 10, 2017 16:45
@nlohmann
Copy link
Owner

That would be great!

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

Successfully merging this pull request may close these issues.

Request: range-based-for over a json-object to expose .first/.second
3 participants