json: Horribly inconsistent behavior between const/non-const reference in operator [] ()

I know I’m late to the discussion here but I wanted to add some thoughts about this. I understand that the idea is that since const operator[] is unchecked for arrays, it should be fine to make it unchecked for maps too. But I don’t think that’s true. There are many reasons why operator[] is unchecked for arrays. But those reasons do not apply to maps.

First of all, indexing into an array is fundamentally a very cheap operation, possibly a single processor instruction. Adding a bounds check to it makes the operation many times slower. By contrast, looking up an entry in a string map is already an expensive operation involving O(log n) string comparisons. Doing an extra it != container.end() check only makes it negligibly slower.

Second of all, the most common operation with input arrays is to iterate through them and access each element. With this usage pattern, it’s easy to ensure that there is no out-of-bounds access. By contrast, with input maps, random access by key is more common than iteration. And with random access it’s a lot more likely that out-of-bounds access is a real possibility that you have to deal with.

Third of all, everyone is already very used to the fact that indexing into arrays is unchecked, ever since the days of C. There is no corresponding expectation for maps. I think that when people are writing code using json objects in C++, they might expect one of 2 things:

1. json objects are like std::map, i.e. there is no const operator[]
2. json objects are like dicts in Python, etc, i.e. operator[] throws for keys that aren’t found

I really don’t think that people’s first thought would be that the behavior of operator[] for json objects is the same as it is for arrays, i.e. undefined behavior for out-of-bounds access. That, to me, is a fundamentally surprising thing about the interface of this library. Of course you can try to remedy the problem of a surprising interface with documentation, but it’s still a problem.

I don’t agree with the concern that it violates “pay only for what you use”. Like I mentioned earlier, the cost of the check is negligible compared to the existing cost of the indexing operation. It’s a small price to pay for eliminating a source of undefined behavior which, in my opinion, an inexperienced user is very likely to run into. Someone said that this check would make the 99.99% case slower for the sake of the 0.01% case. I strongly disagree with that analysis. How can it be that in 99.99% of cases, people are so sure about the source of their input that they’re willing to invoke undefined behavior if they’re wrong? In 99.99% of cases, the json that you’re reading came from some external source, whether a config file or some other process. You really want to give that external source the power to cause a segfault in your application? I think in 99.99% of cases, people are going to want checked access. Or, if they’re an inexperienced developer, they won’t bother to do any checking thinking they don’t need it, until the day that they’re wrong and now they have a segfault to deal with.

In conclusion, what I want is for the const version of operator[] to have the same behavior as at, because operator[] has the nicer, more obvious syntax, so it should be the safe version. Adding this check would even be a backwards-compatible change because you’re taking something that’s undefined behavior now and making it defined behavior.

While we’re here, another possible behavior for const operator[] would be to have an empty instance of basic_json as a static variable in the class, and have const operator[] return a const reference to this instance if the lookup fails. This would be very useful for reading nested configs where all the fields are optional, e.g.:

void loadConfig(const json& config) {
  const json& child = config["foo"];
  a_ = child.value("a", 0);
  b_ = child.value("b", true);
  c_ = child.value("c", "bar");
}

In this case, since all the fields inside of “foo” are optional, we can make “foo” itself be optional.

But probably the less surprising behavior for const operator[] is to throw. The above behavior of returning a const reference to a dummy instance is useful in its own right so it could be added as a new method, say child(key).

There is no need for a special preprocessor symbol, assert comes with one as part of the standard, as I mentioned.

I agree with leaving things as is.