std::unorderd_map cannot be used as ObjectType

[nlohmann]

The following code does not compile:

using json = nlohmann::basic_json<std::unordered_map, std::vector, std::string, bool, int64_t, double, std::allocator>;

The reason for this seems to be that the definition of object_t seems to rely on specifics of std::map:

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

[erichkeane]

I've been looking at this for a bit, and a 1st run solution isn't that bad. Just doing SFINAE/enable_if to switch based on unordered_map vs map is pretty easy using is_same or is_base_of.

However, I'm still trying to figure out a way to not break existing code that specializes the ObjectType parameter to a non-std::map container (such as a custom map that has object_t compatible template args).

The difference between map and unordered_map is 2 fold:
1- Different NUMBER of arguments
2- Different types of arguments. Essentially, the 3rd template param for std::map is a Comparer, whereas in unordered_map this is replaced by a HashFun and KeyEqualityFun.

Optimally, the object_t would be defined entirely based on the ObjectType signature, though I'm currently having a tough time coming up with anything.

[nlohmann]

I think one can check via enable_if whether the container has a function hash_function() to differentiate between std::map (false) and std::unordered_map (true). It's not perfect this way, but it could be a first step.

[erichkeane]

Alright, sounds acceptable. I'm not a HUGE fan of doing it by anything but signature, since the result is overly dependent on the std::map vs std::unordered_map types, but I'll put a pull request together for sdoing it that way in a few hours. Thanks!

[erichkeane]

I've made an attempt that is ALMOST working. I differentiate between the two types via SFINAE based on key_comparer and hasher, which makes object_t do exactly what we need it to.

See the commit here: https://github.com/erichkeane/json/commit/90ab92ade1df8d32ea52e7ad302b41c47b783dce

One might think that type-test-validity would work the same way I'm doing it without having to dive into the member types, however unordered_map will go through resolution successfully with only the 4 template arguments, so it would otherwise be ambiguous.

The calls to void_converter are done with ObjectType<char,char>::... since the Value type is incomplete at the time.

HOWEVER, you'll note if you attempt to build this that this won't compile with an unordered_map due a number of lines requiring object_t::key_type/value_type, which requires the unordered_map to be fully fleshed out, yet basic_json is still incomplete.

The only solution to the problem that I could find is to convert all object_t::key_type to StringType and value_type to std::pair<StringType,basic_json>.
See the commit here: https://github.com/erichkeane/json/commit/16e8d999671e13a2272390acc5b38cd2198e23d6

I'll do a pull request, but I wanted a better explanation here. Also, if there is a nicer solution to the second commit, I'd REALLY love to see it.

[palacaze]

An alternative would be to let the user provide partially defined template types for ObjectType and ArrayType. This is a little more work for the user but makes it easier to use other containers, provided they meet the requirements. Those requirements may be checked through static asserts.

More specifically, the object type should respect the AssociativeContainer requirements, or at the very least the subset you need.

basic_json would then look like that:

template <typename K, typename V>
using defaut_object_type = std::map<K, V>;

template <typename V>
using default_array_type = std::vector<V>;

template <
    template<typename U, typename V> class ObjectType = defaut_object_type,
    template<typename U> class ArrayType = default_array_type,
    class StringType = std::string,
    class BooleanType = bool,
    class NumberIntegerType = int64_t,
    class NumberFloatType = double
    >
class basic_json {

};

I removed the allocator for now, as basic_json can only use stateless allocators right now, it makes sense to provide custom allocators the same way I would define custom containers:

template <typename T>
class my_custom_static_allocator;

using custom_string_type = std::basic_string<
            char, std::char_traits<char>, my_custom_static_allocator<char>>;

template<typename K, typename V>
using custom_object_type = std::unordered_map<K, V, std::hash<K>,
            std::equal_to<K>, my_custom_static_allocator<std::pair<const K, V>>>;

template<typename V>
using custom_array_type = std::vector<V, my_custom_static_allocator<V>>;

using my_json = basic_json<custom_object_type, custom_array_type, custom_string_type>;

Right now for stateless allocators, the create() helper function would need to be reworked in order to use the allocator_type typedef that should be defined for any allocator-aware container.

[nlohmann]

Hi @erichkeane, thanks for your effort! What do you think of @palacaze 's proposal? It may be more verbose, but it could help to approach issue #161 as well.

[erichkeane]

I really like his proposal, it would definitely save on a whole bunch of future specializations, and make sure that we are properly abstracted. My concern was solely that since this is a 1.0.0 product, that changing APIs at all would be prohibitive.

I also like the allocation work that he wishes to do, it doesn't seem like it would be that bad!

My question for @palacaze: How do you see the two working together? If we no longer make basic_json use an allocator parameter (here), how do we get #161 to work as well? Would we want to make the custom object type be something like:

template<typename K, typename V, template<typename> class Alloc>
using cust_obj_type = std::map<K,V, std::hash<K>, std::equal_to<K>, Alloc>;

?

Or is the idea that the custom array/object types would be configured with their OWN allocators that didn't match the rest of the basic_json type?

[palacaze]

Ok so I looked into it and I won't be that easy.

I will mention both #161 and this bug here, first in reply to @erichkeane how do I make basic_json allocator-aware in relation to my previous idea?

I think a judgement call is needed flexibility-wise, it is difficult to get the best of both worlds.

First of all, we need to consider the potential uses of an allocator. It may be used for basic_json internal nodes (through the create() helper) and for data of each container: object, array and string ; so I count 4 cases. (The lexer may also benefit from it).

The use of the user-provided allocator for internal nodes is a no-brainer, this is what is expected and easy to do by supplying the reference to she unique_ptr constructor.
On the other hand, propagating the allocator to every container-type is trickier because it implies that the (possibly custom) containers can use the user-supplied allocator. Allocators have no obligation of being generic in the sense that they may be tailored specially for only one specific data-type. For example we may want to use an allocator optimized for fixed-sized data types. It would perhaps work well with std::list and fail to work at-all for std::vector. How do we fix that ?
(My example is far-fetched because std::list rebinds the allocator in order to allocate internal data nodes.)

1. We can either enforce some rules, maybe declare that basic_json supports custom and stateful allocators but only if they are generic enough to support internal allocation as well as the container types,
2. Or maybe we can state that user supplied allocators are supported but it is up to the user to make sure that the containers can use them and that we give no provision over whether it will be used or not. The trick would be to decide at compile-time if either the supplied allocator should be passed to the containers or not,
3. At last — this one seems crazy — we could leverage std::scoped_allocator_adaptor in order to supply one outer allocator for node-allocation and up to 3 inner allocators, one per container. Well in fact std::scoped_allocator_adaptor is for purely nested containers, here we have a 2-level hierarchy and different container types on the second level, so a now container-manager helper class would be needed, one able to store up to 4 allocator references. I wont expand on this idea because I would not expect people to use this stuff.

So how would I make this work in practise? The first two ideas are quite similar API-wise and mostly differ by their behaviour. Well we provide constructors with an optional allocator parameter.
The pull request #170 is a "somehow-it-compiles" kind of proof of concept implementing the first idea. All the current unit tests but the one checking for standard layout pass, as I added a reference to an allocator. This is seriously untested, I did not even try out with a custom allocator. So this is sort of good news for #161 but as you can see also an quite intrusive change. This is why I am not persuaded basic_json should be made allocator-aware c++11 style.

Unfortunately, I got also some bad news. The array_t and object_t types as defined currently use incomplete template parameter types, and this is not really supported by the STL. In fact using unordered_map does not even compile. @erichkeane met the same problem and got around it by avoiding the use of object_t. The problem is has the potential to be undefined behaviour stuff. Here is a SO thread mentioning this.

The obvious fix would be to store pointers in the map and vector, but it implies a big overhaul of the code. Another would be to introduce your own containers allowing incomplete types. The Boost libraries have them.

[nlohmann]

Reading about all the problems, I am also getting unsure whether it is a good idea to make the class allocator-aware. It was not a design goal in the first place, but (in my limited understanding - which is also wrong as we see in #161) at some point it seems to be an easy addition.

That said, I have limited understanding of the values of a user-provided allocator, so I cannot really judge whether the effort is actually adding value in the end.

[nlohmann]

The fix for this issue is in #209, and the issue with undefined behavior due to incomplete types in std::unordered_map is present again.