Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for unordered_map as object_t #2932

Closed
msarehn opened this issue Aug 13, 2021 · 10 comments
Closed

Support for unordered_map as object_t #2932

msarehn opened this issue Aug 13, 2021 · 10 comments
Labels
kind: bug solution: wontfix the issue will not be fixed (either it is impossible or deemed out of scope) state: help needed the issue needs help to proceed

Comments

@msarehn
Copy link

msarehn commented Aug 13, 2021

Is std::unordered_map supposed to be supported by now? The documentation mentions this (https://json.nlohmann.me/api/basic_json/object_t/#template-parameters), but it still fails to compile.

I know there've been passt issues tagged with wontfix. Just asking because std::map actually seems to be a bottleneck in our use-case and the docs still mention std::unordered_map.

@nlohmann
Copy link
Owner

Unfortunately, using std::unordered_map still does not work, though I have not looked into this issue for a while. Until then, I think I have to adjust the documentation.

@nlohmann
Copy link
Owner

The first template parameter to nlohmann::basic_json is called ObjectType, and is used to define the type to store objects. It has the following default parameter:

template<typename U, typename V, typename... Args> class ObjectType = std::map

Originally, we assumed U to be the key type and V to be the value type (which is nlohmann::basic_json again).

This would allow to pass both std::map and std::unordered_map as first template parameter, because both have at least two template arguments, and agree on the semantics of the first two parameters:

template<
    class Key,
    class T,
    class Compare = std::less<Key>,
    class Allocator = std::allocator<std::pair<const Key, T> >
> class map;

and

template<
    class Key,
    class T,
    class Hash = std::hash<Key>,
    class KeyEqual = std::equal_to<Key>,
    class Allocator = std::allocator< std::pair<const Key, T> >
> class unordered_map;

The problem comes later, when inside basic_json, the type object_t is defined from template parameter ObjectType:

using object_t = ObjectType<
    StringType,
    basic_json,
    object_comparator_t,
    AllocatorType<std::pair<const StringType, basic_json>>
>;

This implicitly assumes that the third template parameter is a comparator and the forth parameter is an allocator. This, however, is only true for std::map.

Note in object_comparator_t we already extracted which comparator to use as C++14 allows for transparent operators:

#if defined(JSON_HAS_CPP_14)
    using object_comparator_t = std::less<>;
#else
    using object_comparator_t = std::less<StringType>;
#endif

So in order to allow std::unordered_map as ObjectT, we need to add a means to properly define object_t from it. I'm not sure how to do this yet, but at least I better understood the reason for the first error the compiler throws at me when I try to compile

using ujson = nlohmann::basic_json<std::unordered_map>;
ujson j;

@msarehn
Copy link
Author

msarehn commented Aug 18, 2021

Yep, that's what I gathered as well.

Now I had an idea: If we want "defaults only" and just use defaults for the hash function, equality comparator and allocator, I thought one could do:

template<class K, class V, class...>
using default_unordered_map = std::unordered_map<K, V>;

using ujson = nlohmann::basic_json<default_unordered_map>;
ujson j;

This gets rid of a number of errors, but complains that ujson is an incomplete type. This I don't quite understand.

@nlohmann
Copy link
Owner

I'm currently trying this:

    using object_t = typename std::conditional<
         true,

         std::map<StringType,
         basic_json,
         object_comparator_t,
         AllocatorType<std::pair<const StringType,
         basic_json>>>,

         std::unordered_map<StringType,
         basic_json,
         std::hash<StringType>,
         std::equal_to<StringType>,
         AllocatorType<std::pair<const StringType,
         basic_json>>>
     >::type;

The upper case is the current default (using std::map), and the lower part is std::unordered_map. Now I'm struggling to replace the true with some std::is_same that compares ObjectType against std::map or std::unordered_map.

@nlohmann nlohmann added the state: help needed the issue needs help to proceed label Aug 18, 2021
@nlohmann
Copy link
Owner

I made some progress (see #2956), but I guess I now hit a blocker: std::unordered_map seems not to allow incomplete types. In particular, a member variable of basic_json should be std::unordered_map<std::string, basic_json> which triggers errors in all compilers.

Any ideas how to fix thisß

@msarehn
Copy link
Author

msarehn commented Aug 22, 2021

No, not really. Other people have the same problem and there doesn't seem to be a proper solution except using an pointer indirection.

@nlohmann
Copy link
Owner

nlohmann commented Sep 2, 2021

Then I guess all i can do is adjust the documentation that std::unordered_map is impossible to use. :-/

@msarehn
Copy link
Author

msarehn commented Sep 3, 2021

Too bad :-( Maybe we can come up with our own container solution that somehow wraps std::unordered_map. If we ever have something, I'll give you a ping. Maybe it'll help 🤷

@nlohmann nlohmann added the solution: wontfix the issue will not be fixed (either it is impossible or deemed out of scope) label Oct 7, 2021
@rimmartin
Copy link

Hi, do you have a branch where you were working on this?

@nlohmann
Copy link
Owner

Hi, do you have a branch where you were working on this?

Yes, see #2956.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug solution: wontfix the issue will not be fixed (either it is impossible or deemed out of scope) state: help needed the issue needs help to proceed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants