simple .each/.forEach extension

[sfinktah]

This is more of a nascent idea, than a ready-to-pull addition. Although it did work perfectly for me, I am not at all sure it would behave well while iterating through a container of mixed data types.

The idea is pretty simple... I do some work on an underscore/c++ fork https://github.com/sfinktah/lodash-cpp which implements the .each method (badly).

I am going to assume you are familiar with underscore/lodash/JavaScript's .forEach, since JSON derived from JavaScript, but I could be wrong.

I'm not at all sure you wish to extend your library in this direction, I only did so because it was beyond my ability to properly handle JSON containers in an external library.

This is what I wanted to achieve, and it worked perfectly for what I required.

        // json j;
        // std::vector<std::string> loadedIpls;

        if (j["unloadScenery"].is_array()) {
            j["unloadScenery"].each([&](std::string ipl, auto, auto) {
                if (IS_IPL_ACTIVE(ipl))
                    REMOVE_IPL(ipl);
                _::pull(loadedIpls, ipl);
                _::each(loadedIpls, [](std::string ipl) { REMOVE_IPL(ipl); });
            });
        }

To me (eye of the beholder) this is beautiful code, so very close to the simplicity of manipulating arrays in JavaScript (using underscore/lodash for extra "simplicity"), expressed succinctly in C++.

If you're not familiar with underscore/lodash, then this probably look horrendous.

The .each method I propose also handles objects (associative containers) with the same ease, something which JavaScript's internal .forEach function never managed, but could be achieved with underscore/lodash.

I'm not sure how it will fare with your CI tests, and obviously it's not worth me pursuing the possibly issues to achieve full functionality, if this simply isn't something you want to add to your library.

[nlohmann]

How is this different to C++'s http://en.cppreference.com/w/cpp/algorithm/for_each?

[coveralls]

Coverage remained the same at 100.0% when pulling 9aff332 on sfinktah:sfinktah-each into cc937de on nlohmann:develop.

[sfinktah]

I'm glad you asked.

std::for_each only passes the value, it doesn't pass the key.

It is possible with STL associative containers to obtain the key via

        for (std::pair<std::string, std::string> i : container) {
            // i.first;  i.second
        }

but that will cause a run time error if you attempt it on a json array/object.

For an array, you could use

        for (auto i = container.begin(); i != container.end(); ++i) {
            function(*i, std::distance(container.begin(), i));
        }

But there was no generic (read; STL compliant) method of iterating the keys in a json container that I could determine.

Although there may be a trick to do it, e.g.,

std::map<std::string, json> tmp = j["container"];

I quite like the idea of a single method handling all types of json containers, it's more in keeping with the spirit of your library.

And it's trivial, but there's a certain beauty in:

            m["root"]["key"]["node"].each([&](const std::string &_, auto, auto) {
                result.push_back(_);
            });

vs

            std::for_each(j["root"]["key"]["node"].begin(), m["root"]["key"]["node"].end(), [&](const std::string& _) {
                result.push_back(_);
            });

[sfinktah]

If you're not sold on the entire .each idea, I could get by with a way to list all the keys in an associative container, then I could iterate by key name.

[gregmarr]

Have you looked at iterator_wrapper, it should do what you need.

[sfinktah]

I have looked at iterator_wrapper, but as far as I can tell, it returns an iterator_wrapper_internal which has

        /// return key of the iterator
        std::string key() const

defined, and as such does not lend itself to a generic STL solution.

You may have ready my unrelated 'issue' (whine) regarding how zen::xml just isn't up to the nlohmann::json standard. It does however offer one option that I put to use to solve the equivalent problem.

    auto itpair = nodeVehicle->getChildren();
    _::each_key_value(itpair.first, itpair.second, [&](auto, auto, auto) {});

[nlohmann]

With the iteration_wrapper, you can use any STL algorithm which relies on iterator pairs. And inside the passed function, you can call key() and value() to access the parts you need. I am confident that whatever usecase each/forEach can handle, an STL algorithm with iteration_wrapper can also do.

[coveralls]

Coverage remained the same at 100.0% when pulling 5e984a2 on sfinktah:sfinktah-each into cc937de on nlohmann:develop.

[nlohmann]

I understand the idea behind this PR, but it feels strange in a C++ world. As the iteration wrapper provides everything needed to solve .forEach and .each scenarios, the PR would not add new functionality.

[sfinktah]

With the iteration_wrapper, you can use any STL algorithm which relies on iterator pairs. And inside the passed function, you can call key() and value() to access the parts you need.

It's the key() and value() parts which cause the issue.

Lets put my PR aside. The only reason I wrote it, was because I couldn't iterate json keys in my lodash-cpp library using standard STL methods. This may simply be a reflection of my ineptitude in template specialization, which is limited to about the level of std::enable_if_has_type<typename T::value_type>, and comparing the type to a nlohmann::json container isn't going to end well if your library isn't present.

I have already mentioned that the well known method for obtaining keys from STL associative containers for (std::pair... ) won't work, but here is another method that was suggested to me by a far more capable programmer.

I've rendered it as a self-contained test. I can put it into your issues queue if you like, it's possible that the solution may be simpler than attempting to make the internal_iterator compliant with .first and .second

#include <map>
#include <string>
#include <vector>
#include <algorithm>
#include "json.hpp"

using json = nlohmann::json;

template <typename ResultContainer, typename Container, typename Function>
ResultContainer map(const Container& container, Function&& function) {
    ResultContainer result;

    std::transform(std::begin(container), std::end(container),
                   std::back_inserter(result), std::forward<Function>(function));
    return result;
}

template <typename ResultContainer, std::size_t I, typename Container>
ResultContainer get_item(const Container& container) {
    return map<ResultContainer>(container, [ ](auto & value) {
        return std::get<I>(value);
    });
}

template <typename ResultContainer, typename Container>
ResultContainer tuple_keys(const Container& container) {
    return get_item<ResultContainer, 0>(container);
}

int main(int argc, const char **argv) {
    std::map<std::string, std::string> m = {
        {"a", "x"},
        {"b", "y"},
        {"c", "z"}
    };

    tuple_keys<std::vector<std::string>>(m);

    json j(m);
    // will not compile with this line uncommented
    // tuple_keys<std::vector<std::string>>(j);
}

[sfinktah]

Same with this, FYI.

    template <typename ResultContainer, typename Container>
    ResultContainer tuple_values(const Container& container)
    {
        return get_item<ResultContainer, 1>(container);
    }

And none of this addresses the fundamental problem of having to know whether a JSON container is an array or an object, in order to obtain the values.

[theodelrieu]

The issue here is that the library doesn't specialize std::get<N>. Note that it cannot specialize std::tuple_size, since the size is not known at compile time.

Even if we define std::get<nlohmann::basic_json<...>>, which type should it return? I would expect std::get to only work on JSON arrays, not objects.

I don't really see the benefit of having a new function in basic_json which would only work for objects (and return an empty container for every other JSON type).

You could add a small utility function json_keys in your library which would do such a thing:

#include <map>
#include <string>
#include <vector>
#include <iostream>
#include <algorithm>
#include "json.hpp"

using json = nlohmann::json;

int main(int argc, char const *argv[])
{
  json j{{"Zero", 0}, {"One", 1}, {"Two", 2}};
  auto wrapper = json::iterator_wrapper(j);
  auto const begin = wrapper.begin();
  auto const end = wrapper.end();

  std::vector<std::string> keys;
  std::transform(begin, end, std::back_inserter(keys),
                 [](auto elem) { return elem.key(); });
  for (auto const& elem : keys)
  {
    std::cout << elem << std::endl;
  }
}

[sfinktah]

I'm going to try a different tack.

There's a section in the README: https://github.com/nlohmann/json#conversion-from-stl-containers

But there's no section for "conversion to stl containers".

Here's what that section could look like:

// converting to a std::map
std::map<std::string, int> m;
j.each( [&] (const std::string& value, const std::string& key) {
    m.emplace({value, key});
};

// converting to a std::vector
std::vector<int> m;
j.each( [&] (int value, auto&) {
    m.push_back(value);
};

// converting to a std::vector (using the key as the index)
std::vector<int> m;
j.each( [&] (int value, int key) {
    m[key] = value; 
};

and, here's one from the a project I'm working on:

std::multimap<uint64_t, uintptr_t> g_hints;
if (j.is_object()) {

    // existing code
    for (auto it = j.begin(); it != j.end(); ++it) {
        uint64_t key = std::stoull(it.key());
        json& values = it.value();
        for (auto it2 = values.begin(); it2 != values.end(); ++it2) {
            uintptr_t value = *it2;
            g_hints.insert(std::make_pair(key, GTA5().base() + value));
        }
    }

    // what it **could** look like
    j.each([&](const json& values, const std::string& _key) {
        uint64_t key = std::stoull(_key);
        values.each([&](uintptr_t value, auto) {
            g_hints.insert({ key, GTA5().base() + value));
        });
    };

    // what it **could** look like in a perfect world
    // if the library would obligue by converting the key
    // (or if our multimap used a string key)

    j.each([&](const json& values, int key) {
        values.each([&](uintptr_t value, auto) {
            g_hints.insert({ key, GTA5().base() + value));
        });
    };
}

Converting from json to a std::multimap in 3 lines of code, how can that not be a good thing?

[sfinktah]

Full disclosure: I actually tested my real-life example, and it required a few small modifications to the PR, and to the code. This is the actual functional code.

j.each([&](json& values, json& _key) {
  uint64_t key = std::stoull(_key.get<std::string>());
    values.each([&](uintptr_t value, auto) {
        g_hints.insert({ key, GTA5().base() + value });
    });
});

[gregmarr]

What about this? It looks very similar and requires no modification to the library. I've compiled this on Compiler Explorer but not run it.

auto jw = json::iterator_wrapper(j);
std::for_each(jw.begin(), jw.end(), [&](auto &x) {
    uint64_t key = std::stoull(x.key());
    std::for_each(x.value().begin(), x.value().end(), [&](auto &value) {
        g_hints.insert({ key, GTA5().base() + value });
    });
});

[gregmarr]

Here is your "conversion to stl containers" section with current develop code, with a fix to the last one to make it work:

// converting to a std::map
std::map<std::string, int> m = j;

// converting to a std::vector
std::vector<int> m = j;

// converting to a std::vector (using the key as the index)
std::vector<int> m;
auto jw = json::iterator_wrapper(j);
std::for_each(jw.begin(), jw.end(), [&](auto &x) {
    auto key = std::stoull(x.key());
    if(key >= m.size()) {
        m.resize(key + 1);
    }
    m[key] = x.value(); 
};

[nlohmann]

I shall have a further look at this after the 3.0.0 release.

[nlohmann]

I think the PR should not be merged for the reasons stated in #848 (comment).

Thanks a lot for the effort though!

[sfinktah]

I want to revisit this general concept now that we're past the 3.0.0 milestone, and aiming at the 4.0.0 milestone.

This time I'm looking at this:

        for (auto& [x, y] : an_stl_map) {} // happy compiler
        for (auto& [x, y] : a_json_instance) {} // sad compiler
        // 'nlohmann320::basic_json<std::map,std::vector,std::string,bool,int64_t,uint64_t,double,std::allocator,nlohmann320::adl_serializer>': 
        // cannot decompose type with non - public members

or

        for (auto i : j.items()) {} // happy compiler
        for (auto [x, y] : j.items()) {} // sad compiler

Which I guess could currently be best achieved by:

        for (const auto& i : j.items()) {
            const auto& x = i.key();
            const auto& y = i.value();
        }

This may be related to #1045 ?

[nlohmann]

@sfinktah I see the issue is still not fixed. Any ideas?

[sfinktah]

I've decided to learn all about iterators.

It seems somewhat unfair to request that something be added, if I'm not prepared to put in the time to research it myself. Especially after seeing how much code (and tickets) were involved in the creation of our current iterator.

I actually still use that custom .each() function I wrote, which has proved quite versatile. The trick now is to replicate that versatility in an iterator.

        // Example of json.each
        j.each([&](json& values, json& key) {
            values.each([&](uintptr_t value, auto) {
                multimap.insert({key, value});
            });
        });

Note how the values are received as json in the outer loop, but are implicitly converted to uint64_t in the inner loop?

That's what I'm hoping to get out of a std::pair (std::tuple?) iterator::value_type... otherwise, I don't really think it's worth the effort.

It should also be possible to rewrite .each() as a std::generator (if they were actually a real C++ thing)...