Open mgorny opened 9 years ago
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.
}
Excuse me but are we talking about the same code? As far as I can see, in JsonConstIterator
operator*
returns Value&
, not a pair…
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 Value
s, and not for object Value
s.
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.
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.
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
.
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.
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.
I second the request, and @daravi 's suggestion seems quite sensible to me.
@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.
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:What I would really prefer is something like this:
which would imply having
keys()
method return some wrapper object withbegin()
andend()
, 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.:Or possibly, even better for my case:
if you don't want to go for wrapper objects :).