beached / daw_json_link

Fast, convenient JSON serialization and parsing in C++
https://beached.github.io/daw_json_link/
Boost Software License 1.0
478 stars 31 forks source link

Compiler error when using custom map type and type alias #428

Closed pkerichang closed 5 months ago

pkerichang commented 5 months ago

Hello,

so I have a custom map/dictionary data structure, from the cookbook, it seems like it is easy enough to just simply make it a type alias of std::unordered_map like so:

namespace daw::json {

template <class Key, class T> struct json_data_contract<my::sorted_map<Key, T>> {
    using map_type = std::unordered_map<Key, T>;
    using class_type = my::sorted_map<Key, T>;

    struct map_builder {
        constexpr class_type operator()(const map_type &val) const {
            class_type ans;
            ans.reserve(val.size());
            for (const auto &[k, v] : val) {
                ans.emplace(k, v);
            }
            return ans;
        }
    };

    using constructor_t = map_builder;

    using type = json_type_alias<map_type>;
    static inline auto to_json_data(const class_type &val) {
        map_type ans;
        ans.reserve(val.size());
        for (const auto &[k, v] : val) {
            ans.emplace(k, v);
        }
        return ans;
    }
};

} // namespace daw::json

however this fails to compile, as it seems to be trying to use daw::construct_a_t to create my::sorted_map from the unordered map, instead of using the map_builder struct I defined. I have to add the following template specialization to get it to work:

namespace daw {

template <class Key, class T> struct construct_a_t<my::sorted_map<Key, T>> {
    constexpr my::sorted_map<Key, T>
    operator()(const std::unordered_map<Key, T> &val) const {
        my::sorted_map<Key, T> ans;
        ans.reserve(val.size());
        for (const auto &[k, v] : val) {
            ans.emplace(k, v);
        }
        return ans;
    }
};

} // namespace daw

Interestingly, I cannot just simply specialize construct_a_t. If I remove the constructor_t typedef and map_builder, compilation also fails. I need to include both for it to work. This seems like an oversight?

beached commented 5 months ago

This might be a bug, looking into why it isn't choosing your specified constructor

beached commented 5 months ago

Just trying to replicate that error. I suspect it is related to the auto deduced mapping and the explicit constructor_t not taking over. I suspect a manual mapping of using type = json_type_alias<json_key_value_no_name<map_type, Key, T, map_builder>>; with a constructor parameter should get you there. However, your map builder will need to take two iterators as param, the type of them is daw::json::json_details::json_parse_kv_class_iterato<...> but it will be easier to take them as a template param e.g.

struct map_builder {
        template<typename Iter>
        constexpr class_type operator()( Iter first, Iter last ) const {
            class_type ans;
            // calling distance here can be costly and incur a parse of the range ans.reserve(val.size());
            // it may be random when the data is out of order from expected but that just preserves the known size

            // alternatively if your map type has an iterators  range ctor return class_type( first, last );
            for( ; first != last; ++first ) {
                auto [k,v] = *first;
                ans.emplace(k, v);
            }
            return ans;
        }
    };
pkerichang commented 5 months ago

I came up with this simple example to test this issue:

template <class Key, class T> class vector_map {
  private:
    using value_type = std::pair<Key, T>;
    using vector_type = std::vector<value_type>;
    using iterator = typename vector_type::iterator;
    vector_type data_;

  public:
    vector_map() = default;

    template <class... Args> std::pair<iterator, bool> emplace(Args &&...args) {
        data_.emplace_back(std::make_pair(std::forward<Args>(args)...));
        return std::make_pair(data_.end() - 1, true);
    }
};

namespace daw::json {
template <class Key, class T> struct json_data_contract<vector_map<Key, T>> {
    using class_type = vector_map<Key, T>;
    using map_type = std::unordered_map<Key, T>;

    struct map_builder {
        template <typename Iter> constexpr class_type operator()(Iter first, Iter last) const {
            class_type ans;
            for (; first != last; ++first) {
                auto [k, v] = *first;
                ans.emplace(k, v);
            }
            return ans;
        }
    };

    using constructor_t = map_builder;
    using type = json_type_alias<json_key_value_no_name<map_type, Key, T, map_builder>>;
    static inline auto to_json_data(const class_type &val) {
        map_type ans;
        ans.reserve(val.size());
        for (const auto &[k, v] : val) {
            ans.emplace(k, v);
        }
        return ans;
    }
};
} // namespace daw::json

void json_test(const std::string &json_file) {
    std::ifstream t(json_file);
    std::stringstream buffer;
    buffer << t.rdbuf();

    auto data_source = buffer.str();
    std::string_view data_view(data_source.data(), data_source.size());

    auto obj = daw::json::from_json<vector_map<std::string, int>>(data_view);
    auto obj_json = daw::json::to_json(
        obj, daw::json::options::output_flags<daw::json::options::SerializationFormat::Pretty>);

    std::cout << obj_json.c_str() << std::endl;
}

however, I get a compiler error I cannot fully understand. What is the correct way to get this working?

beached commented 5 months ago

It looks like once enough of support for container is there, and if the full associative container reqs aren't there that the full json_key_value... stuff needs to be specified it works and round trips. https://github.com/beached/daw_json_link/blob/c5fb56b8954472d0d94f29bf4f99c0039e186477/tests/src/issue_428_test.cpp

pkerichang commented 5 months ago

I'm a little confused by the code for test 428 you posted. Since you added iterator and rvalue constructors to vector_map, why is map_builder still needed? It seems to be redundant now?

also, does this mean that every custom associative containers must have iterator-based and rvalue-based constructors in order for round trip operations to work? There's no way to use the a constructor struct like map_builder to inject those operations? I was hoping I can do that, but if not, that's fine.

Can you clearly describe all the requirements of a custom map container and include complete example code in the documentation?

beached commented 5 months ago

The vector_map above is not fulfilling the associative container requirements. So more needs to be specified. If it did, then using type = json_type_alias<json_key_value_no_name<YourMap>>; should work

beached commented 5 months ago

The minimum is like the following, but begin()/end() are needed, the type aliases for value_type, mapped_type, key_type, the iterator ctor. Maybe a few others, but the following compiles the example.

#include <daw/daw_ensure.h>
#include <daw/json/daw_json_link.h>

#include <iostream>
#include <iterator>
#include <type_traits>
#include <vector>

template<class Key, class T>
struct vector_map {
    using value_type = std::pair<Key, T>;
    using vector_type = std::vector<value_type>;
    using mapped_type = T;
    using key_type = Key;
    using reference = typename vector_type::reference;
    using const_reference = typename vector_type::const_reference;
    using iterator = typename vector_type::iterator;
    using const_iterator = typename vector_type::const_iterator;

private:
    vector_type data_;

public:
    vector_map( ) = default;

    template<typename Iter>
    vector_map( Iter first, Iter last ) {
        while( first != last ) {
            auto [k, v] = *first;
            emplace( std::move( k ), std::move( v ) );
            ++first;
        }
    }
    template<typename Container,
             std::enable_if_t<
               not std::is_same_v<vector_map, daw::remove_cvref_t<Container>>,
               std::nullptr_t> = nullptr>
    vector_map( Container &&values )
      : vector_map( std::begin( values ), std::end( values ) ) {}

    bool operator==( vector_map const &rhs ) const {
        return data_ == rhs.data_;
    }

    const_iterator begin( ) const {
        return data_.begin( );
    }

    const_iterator end( ) const {
        return data_.end( );
    }

    template<class... Args>
    std::pair<iterator, bool> emplace( Args &&...args ) {
        data_.emplace_back( std::make_pair( std::forward<Args>( args )... ) );
        return std::make_pair( data_.end( ) - 1, true );
    }
};

namespace daw::json {
    template<class Key, class T>
    struct json_data_contract<vector_map<Key, T>> {
        using class_type = vector_map<Key, T>;
        using type = json_type_alias<json_key_value_no_name<class_type>>;
    };
} // namespace daw::json

void json_test( std::string_view data_view ) {
    auto obj = daw::json::from_json<vector_map<std::string, int>>( data_view );
    auto obj_json = daw::json::to_json(
      obj, daw::json::options::output_flags<
             daw::json::options::SerializationFormat::Pretty> );

    std::cout << obj_json.c_str( ) << std::endl;
}

int main( ) {
    auto v = vector_map<std::string, int>{ };
    v.emplace( "Hello", 42 );
    v.emplace( "World", 24 );
    auto json_doc = daw::json::to_json( v );
    json_test( json_doc );
    auto v2 = daw::json::from_json<vector_map<std::string, int>>( json_doc );
    daw_ensure( v == v2 );
}
pkerichang commented 5 months ago

Cool, I verified that this works on my end, thanks!