-
-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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
range based for loop for objects #83
Comments
I understand what you mean. I would need to overwork the iterators - I'll check how much effort this would mean. One aspect that needs to be addressed is the fact that the same approach needs to be applicable likewise to arrays. There, it would be cumbersome to access the value with |
Yeah, it would be preferable to not have to access the value of array elements through a std::pair. Do note that in JavaScript, arrays are objects with numeric unsigned monotonically increasing keys. I'm not sure how much of that applies to JSON; it might be weird for the array iterator to be:
ex. Object.keys([0, 1, 2]) // Array [ "0", "1", "2" ] |
This is essentially what I was after in issue #67 and would be happy to see map-like behavior for range based loops. |
Hi @nickdesaulniers and @mlund, I thought about the issue and came to the following idea I would like to discuss with you: I learned the types of the objects that are created in a range-based for are determined by the return value of class basic_json_wrapper
{
public:
// create wrapper objects given a key and a value
inline basic_json_wrapper(const string_t& key, basic_json& value)
: m_key(key), m_value(value)
{}
// implicit type conversion to basic_json
template <class V, typename
std::enable_if<
std::is_convertible<basic_json, V>::value, int>::type
= 0>
inline operator V & ()
{
return m_value;
}
// explicit access to the key
inline string_t& key()
{
return m_key;
}
// explicit access to the value
inline basic_json& value()
{
return m_value;
}
private:
// the key (object key or array index)
string_t m_key = "";
// the basic_json value
basic_json& m_value;
}; This class would behave just like a For objects, I would set The code above is not complete yet - I would need to extend the The code above would allow an example like this: #include "src/json.hpp"
using nlohmann::json;
int main()
{
json foo = {{"one", 1}, {"two", 2}};
for (auto i : foo) {
std::cerr << i << std::endl;
std::cerr << i.key() << std::endl;
std::cerr << i.value() << std::endl;
}
// print: 1, 2
// print: "one", "two"
// print: 1, 2
json bar = {"one", "two", "three"};
for (auto i : bar) {
std::cerr << i << std::endl;
std::cerr << i.key() << std::endl;
std::cerr << i.value() << std::endl;
}
// print: "one", "two", "three"
// print: "0", "1", "2"
// print: "one", "two", "three"
} |
lgtm, but to be consistent with std::map I would prefer i.first and i.second. |
Both have merits; I'm personally in favour of the wrapper due to the cumbersome pair for arrays. std-like behaviour is appealing, though, and I wonder if one could return a for (auto &i : bar) {
string a = i; // "one" ...
string b = i.second; // "one" ...
} The derived template<class T1, class T2, class base=std::pair<T1,T2> >
struct mypair : public base {
mypair(T1 a, T2 b) : base(a,b) {}
operator T2() const { return base::second; }
};
int main()
{
mypair<string,float> v("hej", 1.0);
cout << v << endl; // -> 1.0
} |
I currently see whether I can use an approach similar to |
Neils, I would really appreciate if you check my iterator reimplementation before doing your changes with iterators, if you didn't begin. Otherwise I may have hard time to merge my changes in #88. If you want I may create a separate pull request just for iterators. |
Hi @kepkin, sorry for letting you wait. I did not find the time to look at your PR yet. I really appreciate any work that helps us getting closer to a MSVC build. But since I would like to avoid too many conditionals or other "hacks", I really would like to take some time investigating. I haven't made any changes with respect to this issue yet, and I will not commit anything related to iterators before I checked your code. Right now, I hope to be able to do this on the weekend. |
I checked the implementation of the |
Alright, here would be my implementation: /*!
@brief wrapper to access iterator member functions in range-based for
This class allows to access @ref key() and @ref value() during range-based
for loops. In these loops, a reference to the JSON values is returned, so
there is no access to the underlying iterator.
*/
class iterator_wrapper
{
private:
/// the container to iterate
basic_json& container;
/// the type of the iterator to use while iteration
using json_iterator = decltype(container.begin());
/// internal iterator wrapper
class iterator_wrapper_internal
{
private:
/// the iterator
json_iterator anchor;
/// an index for arrays
size_t array_index = 0;
size_t container_size = 0;
/// calculate a key for the iterator
std::string calculate_key()
{
array_index++;
switch (anchor.m_object->type())
{
/// use integer array index as key
case (value_t::array):
{
return std::to_string(array_index - 1);
}
/// use key from the object
case (value_t::object):
{
return anchor.key();
}
/// use an empty key for all primitive types
default:
{
return "";
}
}
}
public:
/// construct wrapper given an iterator
iterator_wrapper_internal(json_iterator i, size_t s)
: anchor(i), container_size(s), first(calculate_key()), second(*i)
{}
/// dereference operator (needed for range-based for)
iterator_wrapper_internal operator*()
{
return *this;
}
/// increment operator (needed for range-based for)
iterator_wrapper_internal operator++()
{
++anchor;
first = calculate_key();
if (array_index < container_size - 1)
{
second = *anchor;
}
return *this;
}
/// inequality operator (needed for range-based for)
bool operator!= (const iterator_wrapper_internal& o)
{
return anchor != o.anchor;
}
/// stream operator
friend std::ostream& operator<<(std::ostream& o, const iterator_wrapper_internal& w)
{
return o << w.value();
}
/// return key of the iterator
typename basic_json::string_t key() const
{
return first;
}
/// return value of the iterator
basic_json value() const
{
return second;
}
/// public member to simulate std::map::iterator access
typename basic_json::string_t first;
/// public member to simulate std::map::iterator access
basic_json& second;
};
public:
/// construct iterator wrapper from a container
iterator_wrapper(basic_json& cont)
: container(cont)
{}
/// return iterator begin (needed for range-based for)
iterator_wrapper_internal begin()
{
return iterator_wrapper_internal(container.begin(), container.size());
}
/// return iterator end (needed for range-based for)
iterator_wrapper_internal end()
{
return iterator_wrapper_internal(container.end(), container.size());
}
}; To use the wrapper, consider the following example code: #include <json.hpp>
using nlohmann::json;
int main()
{
json j_object = {{"one", 1}, {"two", 2}, {"three", 3}};
json j_array = {"foo", "bar", "baz"};
for (auto i : json::iterator_wrapper(j_object))
{
std::cout << "i.key() = " << i.key() << ", ";
std::cout << "i.first = " << i.first << '\n';
std::cout << "i.value() = " << i.value() << ", ";
std::cout << "i.second = " << i.second << ", ";
std::cout << "*i = " << *i << ", ";
std::cout << "i = " << i << '\n';
}
for (auto i : json::iterator_wrapper(j_array))
{
std::cout << "i.key() = " << i.key() << ", ";
std::cout << "i.first = " << i.first << '\n';
std::cout << "i.value() = " << i.value() << ", ";
std::cout << "i.second = " << i.second << ", ";
std::cout << "*i = " << *i << ", ";
std::cout << "i = " << i << '\n';
}
} The output is
I would be happy for feedback, @nickdesaulniers @mlund @kepkin. |
I think something is wrong with the output? The value of the second and third element should be "bar" and "baz" respectively. Assuming simple copy+paste error, this looks great! Nice job! Maybe a separate issue we can discuss in another ticket; keys in JSON are not sorted, but ordered based on definition. This becomes apparent when iterating keys of an object. |
Hi @nickdesaulniers, thanks for checking by. In fact, there was still a bug in the code. The output should, of course, read:
If no-one objects, I'll push this to master tomorrow. About the ordering of keys: please open an issue. |
Thanks for this — it looks great.
|
🚀 🎸 🍸 🌴 |
There is currently still an issue with the code for the wrapper: The previous builds on Travis all failed due to a segmentation fault. |
Hi. Do you have any plans to add a version of iterator_wrapper which can be constructed from a constant reference? And by the way in my opinion this name, 'iterator_wrapper', is so-so since it's hard to guess what exactly this thing does. One could expect that an instance of this class can be constructed from an iterator, but it can't. I'd call this class 'member_iterator_range' or something like this. |
Hi @vanderlokken, the for (auto v : j.values())
{
} Any better idea? |
Why not the javascript way? for( var prop in obj )
{
var p = obj[prop];
} for( auto prop: obj )
{
json p = obj[prop]
} This would mean the iterator only returns the keys, It would also make it so the syntax for both objects Also note, that this method allows you to have both This could also work without removing any of the other |
@EvilPudding That looks very unnatural in C++. IMO, it should simply be:
|
@xboi209 What would be the type of that obj? If C++ programmers are familiar with iterating through |
@EvilPudding The type of obj would be a custom type defined by this library, but using the map container could work as well. |
@xboi209 I would hate it if it was done, but The problem with this solution is that we would |
I tried to mimic the STL as close as possible. Therefore, JSON objects shall behave as stl::map, and keys/values are available as first/second. However, I don't like how maps behave with range fors, therefore I added the wrapper. |
What part of the map range for behavior do you not like? |
@gregmarr If I do a range for over a JSON object, I only get a reference to the stored object, but have no way to access the key. This is different to when I use iterators explicitly, because I overloaded the iterators with a |
Oh, I thought you meant you didn't like std::map behavior, not json object behavior. |
Ok - all that is left open in this issue would be the name of the wrapper. Does anyone has better ideas? |
Is this issue now defunct and obsolete? I see it's closed - and I don't see the last proposed api available in Release version 2.1.1. I'm trying to simply iterate over the key-value pairs at the leaf-end of a json document -- such that it's simply simple key to basic type value pairs. I can get the values just by doing a range-based for loop, but seems I have no way to get the associated key.
The above works for getting the value, but there seems to be no key accessor... |
It is called for (auto it : json::iterator_wrapper(mode_config))
{
std::cout << "key: " << it.key() << ", value: " << it.value() << std::endl;
} |
Ok! Thank you! That makes sense. I had ended up using classic STL iterators to access it but the range-based for is cleaner and easier to understand. |
For those who find this PR: The way to go is now: for (auto it : mode_config.items())
{
std::cout << "key: " << it.key() << ", value: " << it.value() << std::endl;
} |
Note that the readme recommends this syntax for iterating key/value pairs of an object:
If we try to iterate an nlohmann::json object using a range based for, we only seem to get the value:
prints:
Using some fancyness, it should be possible to provide an API similar to std::map et al for iterating key value pairs of an object.
The text was updated successfully, but these errors were encountered: