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

Key iterator #46

Closed
gnzlbg opened this issue Mar 10, 2015 · 19 comments
Closed

Key iterator #46

gnzlbg opened this issue Mar 10, 2015 · 19 comments

Comments

@gnzlbg
Copy link

gnzlbg commented Mar 10, 2015

When iterating over a json object, e.g. using a range-based for loop, one only gets the values, and it is impossible right now to get the key of each value. It would be nice to be able to iterate over the keys, since this allows you to get the value afterwards if you want too.

Another alternative is a key.value pair iterator like map.

@isaachier
Copy link

The problem with giving access to the key is that not all json objects are composed of key-value pairs. For example, an array is a valid json object but has no keys other than its indices. I have been able to successfully retrieve the keys for a json object by copying the object to a string to object map and iterating through that, though I agree that a more comprehensive solution is necessary.

@isaachier
Copy link

By string to object map I mean string to json object.

@gnzlbg
Copy link
Author

gnzlbg commented Mar 10, 2015

The following is a valid json object: { "b" : {"c" : 2, "d" : 3}, "d" : {"c" : 2, "d" : 3} }.

When iterating over it we get: {"c" : 2, "d" : 3}, {"c" : 2, "d" : 3}, these are just json object themselves.

I would like to somehow be able to get the keys of these objects: "b", "d".

The easiest solution I can think of is for every json object to have a key() method (and maybe also a bool has_key() method) that returns its key, which means the key must be stored inside the json object itself. This includes a memory overhead.

Since you cannot really search the parent object, an alternative more memory-friendly solution is to provide a key-value pair iterator that throws when your object is an array (in the same way that operator[](int) throws when the object is not an array).

@gnzlbg
Copy link
Author

gnzlbg commented Mar 10, 2015

@isaach1000 By string to object map I mean string to json object.

Do you mean std::map<std::string, nlohmann::json> ?

Like this:

  nlohmann::json j
          = {{"b", {{"c", 2}, {"d", 3}}}, {"d", {{"c", 2}, {"d", 3}}}};
  std::map<std::string, nlohmann::json> m = j;
  for (auto&& kv : m) {
      std::cout << kv.first << std::endl;
  }

?

@isaachier
Copy link

The key method used to be part of the iterator class but was recently deprecated. This problem of getting the key is part of a much larger problem, namely that json isn't statically typed but C++ is. This issue is cannot be resolved through inheritance because each type implements a different interface. The best solution I can think of is to allow the user to cast the iterator to a pair and throw an exception if that is invalid.

@gnzlbg
Copy link
Author

gnzlbg commented Mar 10, 2015

The best solution I can think of is to allow the user to cast the iterator to a pair and throw an exception if that is invalid.

Right now operator[](int) throws when used on a {key : value, ...} object sequence; it only works on arrays.

We need an iterator that only works on {key : value, ...} object sequences. A key_value() method on json could return a range with key/value iterators. The method can just throw if the object is an array.

That is, the interface would provide two iterators: one for arrays (return just values), one for object sequences (return key value pairs).

If the previous interface was mixing both into one then I agree that that is a bad idea. You could return something like key-value pair where the key is a variant, and a json object operator[] does something different depending on if the type is a string or the type is an integer:

using key = variant<string, int>;
using value_type = std::pair<key, json>;

for (auto&& kv : json_sequence) {
  auto key = kv.first;
  auto value = kv.second;

  json_sequence[key] = ...;
  }

json operator[](variant<string, int> v) {
  switch(v.which) {
    case integer: { ... }
    case string: { ... }
  }
}

but it is a complex solution.

@isaachier
Copy link

@gnzlbg

Do you mean std::map<std::string, nlohmann::json> ?

Yes

@isaachier
Copy link

@gnzlbg

That is, the interface would provide two iterators: one for arrays (return just values), one for object sequences (return key value pairs).

Eventually, the user needs to decide whether the iterator refers to a key and value or to a key alone. Therefore, I'm suggesting something much simpler to resolve the issue.

Consider this example from the readme:

json j = {
  {"pi", 3.141},
  {"happy", true},
  {"name", "Niels"},
  {"nothing", nullptr},
  {"answer", {
    {"everything", 42}
  }},
  {"list", {1, 0, 2}},
  {"object", {
    {"currency", "USD"},
    {"value", 42.99}
  }}
};

Here is my solution:

auto it1 = j.find("pi");
// it1 is internally represented as a map iterator
auto pair1 = static_cast<PairIter>(it1);

auto it2 = j["list"].begin();
// it2 is internally represented as a vector iterator
auto pair2 = static_cast<PairIter>(it2);
// the above should throw an exception

@gnzlbg
Copy link
Author

gnzlbg commented Mar 10, 2015

@isaach1000 How would you use that with range-based for?

@isaachier
Copy link

Okay that is a good point. How about just having the json object return a pair iterator if it is an object, but a vector iterator if the it is an array?

@nlohmann
Copy link
Owner

Hi there,

an earlier version of the library had key() and value() member functions for the iterators/const_iterators. However, these functions were not available for the reverse_iterators/const_reverse_iterators, because the std::reverse_iterator adaptor seems not to inherit user-defined functions from the iterator class.

There are several ways I can think of approaching the issue:

  1. Add the key() and value() members anyway and live with the fact that they are not available to reverse_iterators.
  2. Do not use the std::reverse_iterator adaptor and manually implement the reverse_iterator/const_reverse_iterator class including the said member functions.
  3. Any magic I cannot think of, but combine the convenience of 2 and the smaller effort of 1. :-)

What do you think, @isaach1000, @gnzlbg?

@gnzlbg
Copy link
Author

gnzlbg commented Mar 24, 2015

You can have your own reverse iterator that inherits from
std::reverse_iterator. You can then forward to the key and value
functions of your non-reversed iterator by using the protected function
current() from std::reverse_iterator, which allows you to get the
underlying iterator.

On Tue, Mar 24, 2015 at 5:48 PM, Niels notifications@github.com wrote:

Hi there,

an earlier version of the library had key() and value() member functions
for the iterators/const_iterators. However, these functions were not
available for the reverse_iterators/const_reverse_iterators, because the
std::reverse_iterator adaptor seems not to inherit user-defined functions
from the iterator class.

There are several ways I can think of approaching the issue:

  1. Add the key() and value() members anyway and live with the fact
    that they are not available to reverse_iterators.
  2. Do not use the std::reverse_iterator adaptor and manually implement
    the reverse_iterator/const_reverse_iterator class including the said member
    functions.
  3. Any magic I cannot think of, but combine the convenience of 2 and
    the smaller effort of 1. :-)

What do you think, @isaach1000 https://github.com/isaach1000, @gnzlbg
https://github.com/gnzlbg?


Reply to this email directly or view it on GitHub
#46 (comment).

@nlohmann
Copy link
Owner

Very good point! I'll check...

@nlohmann nlohmann self-assigned this Mar 24, 2015
nlohmann added a commit that referenced this issue Mar 24, 2015
Currently only support iterator and const_iterator. reverse_iterator
and const_reverse_iterator to be implemented soon.
@nlohmann
Copy link
Owner

I added a key() and value() function to the iterator/const_iterator class. However, I can't manage to inherit from the reverse_iterator class. I tried the following code:

class reverse_foo : public std::reverse_iterator<basic_json::iterator>
{
  public:
    inline typename object_t::key_type key() const
    {
        return base().key();
    }

    inline reference value() const
    {
        return base().operator*();
    }
};

but it seems as if base() is unknown. Any ideas?

@gnzlbg
Copy link
Author

gnzlbg commented Mar 24, 2015

Have you tried this->base() ?

On Tue, Mar 24, 2015 at 7:53 PM, Niels notifications@github.com wrote:

I added a key() and value() function to the iterator/const_iterator
class. However, I can't manage to inherit from the reverse_iterator class.
I tried the following code:

class reverse_foo : public std::reverse_iterator<basic_json::iterator>
{
public:
inline typename object_t::key_type key() const
{
return base().key();
}

inline reference value() const
{
    return base().operator*();
}

};

but it seems as if base() is unknown. Any ideas?


Reply to this email directly or view it on GitHub
#46 (comment).

@gnzlbg
Copy link
Author

gnzlbg commented Mar 24, 2015

With some hindsight now I think that creating your own reverse iterator is
completely unnecessary since those using a reverse iterator can just call
key/value via it->base()->key()/value() right?

That might be problematic when writing generic code, but it is also pretty
easy to write non-member non-friend key(it)/value(it) functions that just
forward the call for normal iterators, and overload them for reverse
iterators. This also would solve the problem, but without needing to create
a special iterator type.

On Tue, Mar 24, 2015 at 9:43 PM, Gonzalo BG gonzalobg88@gmail.com wrote:

Have you tried this->base() ?

On Tue, Mar 24, 2015 at 7:53 PM, Niels notifications@github.com wrote:

I added a key() and value() function to the iterator/const_iterator
class. However, I can't manage to inherit from the reverse_iterator class.
I tried the following code:

class reverse_foo : public std::reverse_iterator<basic_json::iterator>
{
public:
inline typename object_t::key_type key() const
{
return base().key();
}

inline reference value() const
{
    return base().operator*();
}

};

but it seems as if base() is unknown. Any ideas?


Reply to this email directly or view it on GitHub
#46 (comment).

@nlohmann
Copy link
Owner

With this->base() it worked. I committed the changes.

@nlohmann
Copy link
Owner

All iterators have now member functions key() to retrieve the key for objects and value() to retrieve the value. This should fix the mentioned issue.

@gnzlbg
Copy link
Author

gnzlbg commented Mar 26, 2015

Thanks, i'll give it a try :)

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

No branches or pull requests

3 participants