Please provide object key (key,value) iterators

[mgorny]

Right now iterating over objects with ValueIterator pretty much implies iterating over values, with keys being available through non-standard iterator member functions. This is a bit unfriendly, since in order to get all keys (and just keys), I can't use C++11 range-for loops and have to do something like this:

std::unordered_set<std::string> obj_keys;
for (Json::ValueConstIterator it = obj.begin(); it != obj.end(); ++it)
{
    obj_keys.insert(it.name());
}

What I would really prefer is something like this:

std::unordered_set<std::string> obj_keys;
for (const std::string& k : obj.keys())
{
    obj_keys.insert(k);
}

which would imply having keys() method return some wrapper object with begin() and end(), that would in turn give you an iterator over keys.

It would probably also make sense to have some items() method that would return a wrapper for iterator over (key, value) pairs, i.e.:

// not sure about the type signature here :)
for (const std::pair<std::string k, Json::Value v>& : obj.items())
{
}

Or possibly, even better for my case:

std::unordered_set<std::string> obj_keys{obj.keysBegin(), obj.keysEnd()};

if you don't want to go for wrapper objects :).

[BillyDonahue]

In your example:

for (Json::ValueConstIterator it = obj.begin(); it != obj.end(); ++it)
{
}

I don't get it. A few things. I'd spell that Json::Value::const_iterator. Actually, there's no reason not to just call it auto it. But deeper than that, Json::Value::begin() gives you a standard iterator over an internal std::map<Json::CZString, Json::Value>. Why can't you use a ranged-based for loop for that?

for (const auto& kv : obj) {
  const Json::CZString& k = kv.first;
  Json::Value& v = kv.second;
  ...
}

I don't like the API expansion request, because it doesn't feel like Json::Value's job to provide that kind of wrapper. You could certainly use an off-the-shelf iterator adaptor library like Boost.Iterator (http://www.boost.org/doc/libs/1_58_0/libs/iterator/doc/index.html) to get just keys or just mapped values. Json::Value should provide the necessary std::map-like accessors, and let user-level transformations of the iterated sequence be done with extra user-level syntax. I feel like it's already doing the right thing here and the API should stay lean.

I think all you need is a generic (not Json::Value specific) first_view and second_view container wrappers.

for (const auto& k : first_view(obj.begin(), obj.end())) {
    ....  // 'k' iterates over just the keys.
}

[mgorny]

Excuse me but are we talking about the same code? As far as I can see, in JsonConstIterator operator* returns Value&, not a pair…

[BillyDonahue]

Sorry I didn't look closely enough. I was thrown by the example in your question. If JsonConstIterator::operator* returns const Value&, then how would your classic for-loop example get you the keys? Wouldn't that just iterate over values, not keys?

Json::Value::const_iterator makes sense for array Values, and not for object Values.
Value is trying to do a few idioms at once and I imagine it's hard to choose the right behavior sometimes. There should be a way to access a std::pair<const CZString, Value> view of an object Value, but I still think probably not a "keys only" view.

There's a bit of difficulty because the keys are stored as Json::Value::CZString, a private internal detail. They'd have to be exposed to the user as either const std::string& or const char*const or const StaticString or TBD along those lines. The exposed pair would have to be synthesized inside the iterator, and there could be lifetime issues doing so. So a true map-like API for Json::Value is probably a lot of plumbing to do properly. Your idea of keysBegin/keysEnd has the same storage issue and a solution to the storage problem would enable either approach.

[mgorny]

Ok, I've provided a more complete example. Long story short, the iterator has a few extra methods which return the key corresponding to the current value. But as you can guess, for-range loop doesn't expose those methods.

[BillyDonahue]

Ok I see what you mean now. I never looked closely.

I still think that if you can do something without adding members, then do it without adding them.

Range-based for loops are nice, but we shouldn't jump through hoops to make them possible for off-center iteration needs. The range-based for loop can't iterate backwards over a std::vector either, for example. You have to fall back to auto i = std::rbegin(v), e=std::rend(v); i!=e; ++i) which ain't so bad. Perhaps analogously:

for (auto i = beginNameIter(obj), e = endNameIter(obj); i!=e; ++i)

could work. These would return iterator adaptors that call .name() in their deref operator.

PS: There are a number of other little problems with ValueIteratorBase and its derived classes. It has an internal typedef to size_t that should be size_type. Its relationals are members. const shouldn't be part of value_type.

[cdunn2001]

Because, as @BillyDonahue mentioned, Value can be many kinds of object, I would oppose adding methods which behave differently for different types. So I favor adaptors. You would have to know what kind of object you have:

std::unique_ptr<Json::CharReader> reader = ...;

Json::Value valA = Json::parseFromString(reader, R"([1, 2, 3])");
Json::ArrayAdaptor const ary(&val);
for (auto& key : ary) ...;

Json::Value valB = Json::parseFromString(reader, R"({"x": 1, "y": 2})");
Json::ObjectAdaptor const obj(&val);
for (auto& kv : obj) ...;
for (auto& k : obj.keys()) ...;
for (auto& v : obj.values()) ...;

I've used ArrayAdaptor instead of ArrayValue because they do not have deep-copy semantics -- the underlying Value needs to exist -- and they do not have all the methods of Value.

The Adaptors can check data-types:

Json::Value valA = Json::parseFromString(reader, R"([1, 2, 3])");
Json::ObjectAdaptor const ary(&val); // throws exception

They would be non-virtual, so methods could be added easily.

[daravi]

Hi just wondering if there are any updates on this ticket? It would be great if I could use adapters or if I could do something like this:

Json::Value v = getSampleJsonValue();
if (v.isObject())
{
    for (auto& [key, val] : v.asObject())
    {
        // use key (std::string&), and val (Json::Value&)
    }
}
if (v.isArray())
{
    for (auto& val : v.asArray())
    {
        // use val (Json::Value&)
    }
}

I think having explicit asObject and asArray methods would be nicer and make the API easier to remember.

[timomrs]

I second the request, and @daravi 's suggestion seems quite sensible to me.

[bcsgh]

@BillyDonahue

As of 2024 "Range-based for loops" are not just "nice", but the default for anything container like and expecting user to "jump through hoops" for center of the road iteration is what I'd, at this point, call an API defect rather than an enhancement request.

If the difference between iterating over an array vs an object is a big deal, then something that manifests as pair<Value, Value> might be justified. Or maybe provide obj.arrayItems() and obj.items() (where the first indexes as integers and the second renders integer indexes as strings) could be included.