enable as<std::map>() use if key OR value has as() implementation

[mdrohmann]

In a C++11 project, I based my msgpack code on the example from cpp11/non_def_non_class. I believe it is very elegant, as the only thing I have to do to adapt custom classes is implementing an as<>() method. That is really lovely!

Unfortunately, my code broke (tries to use the deleted default constructor) when I tried do the following

 obj.as<std::map<int, my> >()

The reason is that msgpack-C++ is not using the as<std::map>() implementation, but falls back to the convert<std::map>() implementation, because has_as<int> is false. The proposed pull request fixes the problem, and adapts the non_def_non_class example to demonstrate the problem and its fix.

[redboltz]

Thank you for sending the PR. I will check it on this weekend.

[redboltz]

It is not good. Even if the type K or V doesn't have as, the map<K, V> matched as.

[redboltz]

@mdrohmann , I was wrong. I didn't understand what your fix mean at that time. Now, I understand it. Your fix is correct and simple :)

[redboltz]

msgpack-c supports the map that both key and value doesn't have default constructor. See
https://github.com/msgpack/msgpack-c/blob/master/test/msgpack_cpp11.cpp#L615

In your case, int is not mached to as. The problem is not map but int.

I will add int specilization to as template as follows:

template <>
struct as<int> {
    int operator()(msgpack::object const& o) const {
        return // get int from o
    }
};

If you test your code quickly, insert and implement the above code in your code.

After adding the specialization, obj.as<std::map<int, my> >() compiled successfully.

I will wrote the pull request to add the specialization.

By the way, I'd like to keep the example simple. no_def_con_class.cpp is only the example for the class that doesn't have default constructor.

[redboltz]

I said

The problem is not map but int.

but I noticed that I should modify map, not int now.

My new idea as follows:

Now, there is one as specialization of map

template <typename K, typename V, typename Compare, typename Alloc>
struct as<
    std::map<K, V, Compare, Alloc>,
    typename std::enable_if<msgpack::has_as<K>::value && msgpack::has_as<V>::value>::type> {
    std::map<K, V, Compare, Alloc> operator()(msgpack::object const& o) const {
        if (o.type != msgpack::type::MAP) { throw msgpack::type_error(); }
        msgpack::object_kv* p(o.via.map.ptr);
        msgpack::object_kv* const pend(o.via.map.ptr + o.via.map.size);
        std::map<K, V, Compare, Alloc> v;
        for (; p != pend; ++p) {
            v.emplace(p->key.as<K>(), p->val.as<V>());
        }
        return v;
    }
};

I add the following three specializations:

template <typename K, typename V, typename Compare, typename Alloc>
struct as<
    std::map<K, V, Compare, Alloc>,
    typename std::enable_if<!msgpack::has_as<K>::value && std::is_default_constructible<K>::value && msgpack::has_as<V>::value>::type> {
    std::map<K, V, Compare, Alloc> operator()(msgpack::object const& o) const {
        if (o.type != msgpack::type::MAP) { throw msgpack::type_error(); }
        msgpack::object_kv* p(o.via.map.ptr);
        msgpack::object_kv* const pend(o.via.map.ptr + o.via.map.size);
        std::map<K, V, Compare, Alloc> v;
        for (; p != pend; ++p) {
            K key;
            p->key.convert(key);
            v.emplace(key, p->val.as<V>());
        }
        return v;
    }
};

template <typename K, typename V, typename Compare, typename Alloc>
struct as<
    std::map<K, V, Compare, Alloc>,
    typename std::enable_if<msgpack::has_as<K>::value && !msgpack::has_as<V>::value && std::is_default_constructible<V>::value>::type> {
    std::map<K, V, Compare, Alloc> operator()(msgpack::object const& o) const {
        if (o.type != msgpack::type::MAP) { throw msgpack::type_error(); }
        msgpack::object_kv* p(o.via.map.ptr);
        msgpack::object_kv* const pend(o.via.map.ptr + o.via.map.size);
        std::map<K, V, Compare, Alloc> v;
        for (; p != pend; ++p) {
            V value;
            p->val.convert(value);
            v.emplace(p->key.as<K>(), value);
        }
        return v;
    }
};

template <typename K, typename V, typename Compare, typename Alloc>
struct as<
    std::map<K, V, Compare, Alloc>,
    typename std::enable_if<!msgpack::has_as<K>::value && std::is_default_constructible<K>::value && !msgpack::has_as<V>::value && std::is_default_constructible<V>::value>::type>::type> {
    std::map<K, V, Compare, Alloc> operator()(msgpack::object const& o) const {
        if (o.type != msgpack::type::MAP) { throw msgpack::type_error(); }
        msgpack::object_kv* p(o.via.map.ptr);
        msgpack::object_kv* const pend(o.via.map.ptr + o.via.map.size);
        std::map<K, V, Compare, Alloc> v;
        for (; p != pend; ++p) {
            K key;
            V value;
            p->key.convert(key);
            p->val.convert(value);
            v.emplace(key, value);
        }
        return v;
    }
};

Those four specializations are MECE.

The policy is

• All collections that proved as specialization should satisfy
  • If members have as specialization, then use it.
  • Else if members are default constructible, the collection as specialization should provide convert fallback using default constructor.
  • Else ( some members don't have asspecialization and don't default constructible ) the collections as specialization should be disabled (by SFINAE).

[redboltz]

@nobu-k, what do you think about my idea?

[redboltz]

@mdrohmann , could you test your code?

Adding three specializations to include/msgpack/v1/adaptor/map.hpp just after the first specialization.

[redboltz]

I updated my solution. The first idea was adding tree specializations. I realized that the implementation can be reused. Because as<K>() itself can fallback to convert. So I updated my code just modifying the expression of enable_if.

template <typename K, typename V, typename Compare, typename Alloc>
struct as<
    std::map<K, V, Compare, Alloc>,
    typename std::enable_if<
        (msgpack::has_as<K>::value && msgpack::has_as<V>::value) ||
        (msgpack::has_as<K>::value && std::is_default_constructible<V>::value) ||
        (std::is_default_constructible<K>::value && msgpack::has_as<V>::value) ||
        (std::is_default_constructible<K>::value && std::is_default_constructible<V>::value)
    >::type> {
    std::map<K, V, Compare, Alloc> operator()(msgpack::object const& o) const {
        if (o.type != msgpack::type::MAP) { throw msgpack::type_error(); }
        msgpack::object_kv* p(o.via.map.ptr);
        msgpack::object_kv* const pend(o.via.map.ptr + o.via.map.size);
        std::map<K, V, Compare, Alloc> v;
        for (; p != pend; ++p) {
            v.emplace(p->key.as<K>(), p->val.as<V>());
        }
        return v;
    }
};

[redboltz]

@mdrohmann , as I commented in your code, I understand what does your fix mean now. I will merge your PR, so I withdraw my approach.
I will add some tests for your fix based on example you modified and remove example.

[redboltz]

@mdrohmann , I merged your fix, in addition, I replaced AND with OR other containers. Also I added tests.

Thank you for your cooperation :)

[mdrohmann]

@redboltz I think implementing the alternative as<> implementations for map, would have worked as well, but my original suggestions was simpler probably. As every object has an 'as<>' implementation that falls back to 'convert()' it should always work... It is probably a good idea to remove my code from the example file to keep it simple. I just decided to put it in there in order make it easy for you to see the problem.

Thanks for maintaining building this great library.

[redboltz]

@redboltz I think implementing the alternative as<> implementations for map, would have worked as well, but my original suggestions was simpler probably. As every object has an 'as<>' implementation that falls back to 'convert()' it should always work...

@mdrohmann ,I completely agree with you. When I had seen your PR for the first time, I had forgotten the fall back mechanism. Although I designed it ;)