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.

Sign up for GitHub

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

Jump to bottom

Add support for deserialization of STL containers of non-default constructable types (fixes #2574). #2576

Merged
merged 18 commits into from
Apr 25, 2021
Merged

Add support for deserialization of STL containers of non-default constructable types (fixes #2574). #2576

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
1e825e4
Add support for deserialization of STL containers of non-default cons…
AnthonyVH Jan 8, 2021
c0a8b45
Renamed template parameter and added some comments.
AnthonyVH Jan 9, 2021
1b113f7
Added extra tests to improve coverage.
AnthonyVH Jan 9, 2021
23f462b
Reduced code duplication, renamed tag to identity_tag.
AnthonyVH Jan 10, 2021
672e8bf
Fixed std::pair trying to deserialize via array functions.
AnthonyVH Jan 11, 2021
6ebf274
Add internal version of is_default_constructible to work around LWG 2…
AnthonyVH Jan 12, 2021
6278f31
Simplify from_json overloads.
AnthonyVH Jan 12, 2021
6ef1614
Refactor json::get() to use priority_tag.
AnthonyVH Jan 12, 2021
fc8c584
Regenerated single include.
AnthonyVH Jan 12, 2021
fbf6df6
Enable member function calls in trailing return decltype expressions …
AnthonyVH Jan 12, 2021
d7c0f15
Merged from_json for pair and tuple to try to fix C2995 error in old …
AnthonyVH Jan 13, 2021
6eb37e9
Only add conditional constexpr to get() for >= C++14 to work around e…
AnthonyVH Jan 14, 2021
848927a
Updated comments as requested.
AnthonyVH Jan 23, 2021
130382f
Remove comment about GCC commit which didn't really relate to the code.
AnthonyVH Jan 24, 2021
333612c
Merge remote-tracking branch 'upstream/develop' into non_default_cons…
AnthonyVH Mar 24, 2021
322bc99
Reran amalgamate.
AnthonyVH Mar 24, 2021
8e79917
Fix clang-tidy complaints.
AnthonyVH Mar 24, 2021
2b86513
Fixing CI errors.
AnthonyVH Mar 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 18 additions & 9 deletions include/nlohmann/adl_serializer.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,16 @@
#pragma once

#include <type_traits>
#include <utility>

#include <nlohmann/detail/conversions/from_json.hpp>
#include <nlohmann/detail/conversions/to_json.hpp>
#include <nlohmann/detail/meta/type_traits.hpp>

namespace nlohmann
{

template<typename, typename>
template<typename ValueType, typename>
struct adl_serializer
{
/*!
Expand All @@ -20,14 +22,22 @@ struct adl_serializer
@param[in] j JSON value to read from
@param[in,out] val value to write to
*/
template<typename BasicJsonType, typename ValueType>
static auto from_json(BasicJsonType&& j, ValueType& val) noexcept(
template<typename BasicJsonType, typename U = ValueType>
AnthonyVH marked this conversation as resolved.
Show resolved Hide resolved
static auto from_json(BasicJsonType && j, U& val) noexcept(
noexcept(::nlohmann::from_json(std::forward<BasicJsonType>(j), val)))
-> decltype(::nlohmann::from_json(std::forward<BasicJsonType>(j), val), void())
{
::nlohmann::from_json(std::forward<BasicJsonType>(j), val);
}

template<typename BasicJsonType, typename U = ValueType>
AnthonyVH marked this conversation as resolved.
Show resolved Hide resolved
static auto from_json(BasicJsonType && j) noexcept(
noexcept(::nlohmann::from_json(std::forward<BasicJsonType>(j), detail::tag<U> {})))
-> decltype(::nlohmann::from_json(std::forward<BasicJsonType>(j), detail::tag<U> {}))
{
return ::nlohmann::from_json(std::forward<BasicJsonType>(j), detail::tag<U> {});
}

/*!
@brief convert any value type to a JSON value

Expand All @@ -37,13 +47,12 @@ struct adl_serializer
@param[in,out] j JSON value to write to
@param[in] val value to read from
*/
template<typename BasicJsonType, typename ValueType>
static auto to_json(BasicJsonType& j, ValueType&& val) noexcept(
noexcept(::nlohmann::to_json(j, std::forward<ValueType>(val))))
-> decltype(::nlohmann::to_json(j, std::forward<ValueType>(val)), void())
template<typename BasicJsonType, typename U = ValueType>
static auto to_json(BasicJsonType& j, U && val) noexcept(
noexcept(::nlohmann::to_json(j, std::forward<U>(val))))
-> decltype(::nlohmann::to_json(j, std::forward<U>(val)), void())
{
::nlohmann::to_json(j, std::forward<ValueType>(val));
::nlohmann::to_json(j, std::forward<U>(val));
}
};

} // namespace nlohmann
95 changes: 87 additions & 8 deletions include/nlohmann/detail/conversions/from_json.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#include <nlohmann/detail/exceptions.hpp>
#include <nlohmann/detail/macro_scope.hpp>
#include <nlohmann/detail/meta/cpp_future.hpp>
#include <nlohmann/detail/meta/tag.hpp>
#include <nlohmann/detail/meta/type_traits.hpp>
#include <nlohmann/detail/value_t.hpp>

Expand Down Expand Up @@ -248,6 +249,27 @@ void())
from_json_array_impl(j, arr, priority_tag<3> {});
}

template < typename BasicJsonType, typename Array, std::size_t... Is >
Array from_json_array_impl(BasicJsonType&& j, tag<Array> /*unused*/, index_sequence<Is...> /*unused*/)
{
return { std::forward<BasicJsonType>(j).at(Is).template get<typename Array::value_type>()... };
}

template < typename BasicJsonType, typename T, std::size_t N,
enable_if_t < !std::is_default_constructible<std::array<T, N>>::value, int > = 0 >
auto from_json(BasicJsonType && j, tag<std::array<T, N>> t)
-> decltype(j.template get<T>(),
from_json_array_impl(std::forward<BasicJsonType>(j), t, make_index_sequence<N> {}))
{
if (JSON_HEDLEY_UNLIKELY(!j.is_array()))
{
JSON_THROW(type_error::create(302, "type must be array, but is " +
std::string(j.type_name())));
}

return from_json_array_impl(std::forward<BasicJsonType>(j), t, make_index_sequence<N> {});
}

template<typename BasicJsonType>
void from_json(const BasicJsonType& j, typename BasicJsonType::binary_t& bin)
{
Expand Down Expand Up @@ -323,22 +345,71 @@ void from_json(const BasicJsonType& j, ArithmeticType& val)
}
}

template<typename BasicJsonType, typename A1, typename A2>
void from_json(const BasicJsonType& j, std::pair<A1, A2>& p)
template<typename BasicJsonType, typename A1, typename A2,
enable_if_t<std::is_default_constructible<std::pair<A1, A2>>::value, int> = 0>
void from_json(BasicJsonType && j, std::pair<A1, A2>& p)
{
if (JSON_HEDLEY_UNLIKELY(!j.is_array()))
{
JSON_THROW(type_error::create(302, "type must be array, but is " +
std::string(j.type_name())));
}

p = {std::forward<BasicJsonType>(j).at(0).template get<A1>(),
std::forward<BasicJsonType>(j).at(1).template get<A2>()
};
}

template < typename BasicJsonType, class A1, class A2,
enable_if_t < !std::is_default_constructible<std::pair<A1, A2>>::value, int > = 0 >
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to keep a single from_json overload in these cases, and forward to from_json_pair_impl, which use a priority_tag to ease the SFINAE pain, and to make it possible to add from_json_pair_impl overloads in the future.

Also, removing the SFINAE for overloads with detail::tag will allow default-constructible types to be passed as well, thus reducing code duplication (the first overload can call the second now, instead of copy-pasting)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would the signature of this from_json function be? The second argument could be either std::pair<A1, A2> or identity_tag<std::pair<A1, A2>>, and I'm not seeing how to automatically deduce A1 and A2 when the second argument is e.g. PairRelatedType.

Or do you mean to have a single overload taking an std::pair and one taking identity_tag<std::pair> and then both of those dispatching to from_json_pair_impl? In that case, if the SFINAE is removed for the detail::tag overloads, then there will be two possible overloads available for adl_serializer to call (in case of default constructable types), so then that won't compile anymore due to ambiguity.

I'm probably just overlooking something...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean something like this: https://github.com/theodelrieu/mgs/blob/master/codecs/include/mgs/codecs/basic_codec.hpp#L109

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've pushed an update which removes some code duplication and uses priority_tag to dispatch to the preferred function. Is this what you had in mind?

Note that adl_serializer now potentially has two from_json functions. This is e.g. the case for all default-constructable std::array, std::pair, and std::tuple. This doesn't cause ambiguity because json::get() uses SFINAE to select the "object returning" from_json() in that case (discarded get() vs chosen get()). This choice seems pretty deliberate, and looking at the code it's a logical choice due to possible bypassing at least 1 move construction (at least pre-C++17, although with optimizations there's probably no difference).

While doing some testing I did notice that the "chosen" get() gets called when deserializing a std::vector<float>. I found that quite surprising. I get how this adl_serializer::from_json() could now exist due to my code changes, but looking at the existing code it's not clear to me which detail::from_json would get called in that case. Just something I found odd which I figured was worth mentioning.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this what you had in mind?

Yes, you can remove this line for the priority_tag<0> overload though:

enable_if_t < !std::is_default_constructible<std::array<T, N>>::value, int > = 0 >

This is the point of priority_tag<0>, it forces priority inside the overload set so you can just keep the is_default_constructible check for std::pair<A1, A2>.

Note that adl_serializer now potentially has two from_json functions.

IIRC (it's been a while since I've looked at the code), there is support for such overloads for adl_serializer specializations. I don't see why there would not be one for the base template indeed.

While doing some testing I did notice that the "chosen" get() gets called when deserializing a std::vector.

Seems like a change of behavior. To be honest, the template code is quite dusty, I've learned quite a lot since I wrote it, and it's quite hard to figure out all that's going on inside...

For instance, the different get() overloads would be a nice case for priority_tag. All the rules could be refined using emulated concepts.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated some more code and fixed a potential issue with my std::pair deserialization. Is this OK?

std::pair<A1, A2> from_json(BasicJsonType && j, tag<std::pair<A1, A2>> /*unused*/)
{
if (JSON_HEDLEY_UNLIKELY(!j.is_array()))
{
JSON_THROW(type_error::create(302, "type must be array, but is " +
std::string(j.type_name())));
}

return {std::forward<BasicJsonType>(j).at(0).template get<A1>(),
std::forward<BasicJsonType>(j).at(1).template get<A2>()};
}

template<typename BasicJsonType, typename Tuple, std::size_t... Idx>
void from_json_tuple_impl(BasicJsonType&& j, Tuple& t, index_sequence<Idx...> /*unused*/)
{
p = {j.at(0).template get<A1>(), j.at(1).template get<A2>()};
t = std::make_tuple(std::forward<BasicJsonType>(j).at(Idx).template get<typename std::tuple_element<Idx, Tuple>::type>()...);
}

template<typename BasicJsonType, typename... Args,
enable_if_t < std::is_default_constructible<std::tuple<Args...>>::value, int > = 0 >
void from_json(BasicJsonType && j, std::tuple<Args...>& t)
{
if (JSON_HEDLEY_UNLIKELY(!j.is_array()))
{
JSON_THROW(type_error::create(302, "type must be array, but is " +
std::string(j.type_name())));
}

from_json_tuple_impl(std::forward<BasicJsonType>(j), t, index_sequence_for<Args...> {});
}

template<typename BasicJsonType, typename Tuple, std::size_t... Idx>
void from_json_tuple_impl(const BasicJsonType& j, Tuple& t, index_sequence<Idx...> /*unused*/)
Tuple from_json_tuple_impl(BasicJsonType&& j, tag<Tuple> /*unused*/, index_sequence<Idx...> /*unused*/)
{
t = std::make_tuple(j.at(Idx).template get<typename std::tuple_element<Idx, Tuple>::type>()...);
return std::make_tuple(std::forward<BasicJsonType>(j).at(Idx).template get<typename std::tuple_element<Idx, Tuple>::type>()...);
}

template<typename BasicJsonType, typename... Args>
void from_json(const BasicJsonType& j, std::tuple<Args...>& t)
template < typename BasicJsonType, typename... Args,
enable_if_t < !std::is_default_constructible<std::tuple<Args...>>::value, int > = 0 >
std::tuple<Args...> from_json(BasicJsonType && j, tag<std::tuple<Args...>> t)
{
from_json_tuple_impl(j, t, index_sequence_for<Args...> {});
if (JSON_HEDLEY_UNLIKELY(!j.is_array()))
{
JSON_THROW(type_error::create(302, "type must be array, but is " +
std::string(j.type_name())));
}

return from_json_tuple_impl(std::forward<BasicJsonType>(j), t, index_sequence_for<Args...> {});
}

template < typename BasicJsonType, typename Key, typename Value, typename Compare, typename Allocator,
Expand Down Expand Up @@ -390,6 +461,14 @@ struct from_json_fn
{
return from_json(j, val);
}

template<typename BasicJsonType, typename T>
AnthonyVH marked this conversation as resolved.
Show resolved Hide resolved
auto operator()(const BasicJsonType& j, detail::tag<T> t) const
noexcept(noexcept(from_json(j, t)))
-> decltype(from_json(j, t))
{
return from_json(j, t);
}
AnthonyVH marked this conversation as resolved.
Show resolved Hide resolved
};
} // namespace detail

Expand Down
10 changes: 10 additions & 0 deletions include/nlohmann/detail/meta/tag.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
#pragma once

namespace nlohmann
{
namespace detail
{
// dispatching helper struct
template <class T> struct tag {};
AnthonyVH marked this conversation as resolved.
Show resolved Hide resolved
} // namespace detail
} // namespace nlohmann
3 changes: 1 addition & 2 deletions include/nlohmann/detail/meta/type_traits.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,8 +106,7 @@ struct is_getable
};

template<typename BasicJsonType, typename T>
struct has_from_json < BasicJsonType, T,
enable_if_t < !is_basic_json<T>::value >>
struct has_from_json < BasicJsonType, T, enable_if_t < !is_basic_json<T>::value >>
{
using serializer = typename BasicJsonType::template json_serializer<T, void>;

Expand Down
Loading