accessing key by reference

[crusader-mike]

Is there any way to access key in object by reference? Currently (const_)iterator::key() returns string by value causing extra copy every time I need to access it.

Is there any reason why that string gets returned by value?

[nlohmann]

I'll check.

[nlohmann]

This is the function:

typename object_t::key_type key() const
{
    assert(m_object != nullptr);

    if (JSON_LIKELY(m_object->is_object()))
    {
        return m_it.object_iterator->first;
    }

    JSON_THROW(invalid_iterator::create(207, "cannot use key() for non-object iterators"));
}

m_it.object_iterator is of type std::map<std::string, basic_json>::iterator. I am not sure whether I can access the string in the pair *m_it.object_iterator by reference. Or did I miss something?

[crusader-mike]

Yes, I saw this function. Yes, you can return a reference to std::map key here. I bet it returns by value because there is another key() implementation for array that returns index id converted to string, but (unless there something prevents it) you should be able to have one key() returning std::string const& and another -- std::string.

[nlohmann]

@crusader-mike What do you think of 90eb0a9?

[crusader-mike]

I think proper solution is to have key() method returning (a reference to) empty string for everything but objects and index() method returning size_t (0 for everything but arrays). This would remove the need for array_index_str. In fact, I'd probably make it illegal to call key()/index() on wrong type -- i.e. throw an exception if it happens. This would save me from having to deal with empty_str object and having to decide which value to return from index() call on object type.

But it is probably too late as this is breaking change API-wise... Now going back to least invasive approach you were considering -- it looks ok with few notes:

• I'd use C++11 union (either nothing or index+array_index_str or empty_str). Though be careful about union-related bug in compilers
• I'd populate array_index_str in key(), not in operator++() and maybe even consider doing this only if array_index has changes since last key() call (probably not -- it is unlikely someone will call key() multiple times without changing iterator)
• I'd consider making empty_str static

[crusader-mike]

I think proper solution is to have ... index() method returning size_t (0 for everything but arrays).

on second thought you don't really need index() -- it can be trivially calculated by user as it - j.begin() (assuming that iterator for arrays is a random_iterator)

[nlohmann]

You are right: it makes more sense to only calculate the array key when it is actually accessed. I implemented that. Regarding your other remarks:

• Making empty_str static did not work for me. I am not sure how to realize this in a header-only library.
• Calculating the index from the begin iterator would require passing the reference to iteration_proxy_internal. I'm not sure whether this is worth the effort for saving one std::size_t.
• I have the same feelings for a union: it adds complexity where I'm not sure it is worth it.

[crusader-mike]

Making empty_str static did not work for me. I am not sure how to realize this in a header-only library.

There are few ways, for example:

std::string const& key() const
{
    static std::string empty;
    ...
    return empty;
}

or you could do it via in template class:

template<class> struct S_ { static std::string const empty; };
template<class T> std::string const S_<T>::empty;
using S = S_<void>;    // You can use `S::empty`.

or in C++17 you could use inline variables:

class S { static inline std::string const empty; };

or you could convince your MSVC linker to discard all but first empty_str symbol it sees (GCC has similar concept of "weak" symbols, same for most compilers):

__declspec(selectany) std::string const empty_str;

Calculating the index from the begin iterator would require passing the reference

No, you misunderstood me -- idea was to remove array-related logic from key() altogether. On the grounds that user can find index of array element (his iterator it points to) by using it - j.begin() (and convert to string if he needs to). For this to work your iterator proxy needs to behave as random access iterator if it points into an array. I didn't did in proxy iterator implementation, but I assume it isn't the case. Plus, it is part of API breaking change -- I know how library owners are reluctant to do those, even when it makes sense ;)

I have the same feelings for a union: it adds complexity where I'm not sure it is worth it.

Well, it isn't really complex -- just gotta know about the bug and be careful in owner type constructor.

[crusader-mike]

... followup thought -- instead of trying to "fix" key() it makes sense to simply add new function string const& name() const that returns name of object's element (by reference) and throws if iterator points into array/etc? ... or simply declare that name()'s behaviour is undefined in such situation -- in this way name() can be declared noexcept.

[nlohmann]

Well, I don't want to add a breaking API change for this. I shall merge the feature branch and would be happy about pull requests.

[crusader-mike]

adding name() (per description) wouldn't break anything