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

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
25 changes: 15 additions & 10 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 @@ -249,25 +249,31 @@ 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, identity_tag<Array> /*unused*/, index_sequence<Is...> /*unused*/)
template < typename T, typename BasicJsonType, typename ArrayType, std::size_t... Idx>
ArrayType from_json_inplace_array_impl_base(BasicJsonType&& j, identity_tag<ArrayType> /*unused*/,
index_sequence<Idx...> /*unused*/)
{
return { std::forward<BasicJsonType>(j).at(Is).template get<typename Array::value_type>()... };
return { std::forward<BasicJsonType>(j).at(Idx).template get<T>()... };
}

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, identity_tag<std::array<T, N>> tag)
-> decltype(j.template get<T>(),
from_json_array_impl(std::forward<BasicJsonType>(j), tag, make_index_sequence<N> {}))
template < typename BasicJsonType, typename T, std::size_t N >
auto from_json_inplace_array_impl(BasicJsonType&& j, identity_tag<std::array<T, N>> tag, priority_tag<0> /*unused*/)
-> decltype(from_json_inplace_array_impl_base<T>(std::forward<BasicJsonType>(j), tag, make_index_sequence<N> {}))
{
return from_json_inplace_array_impl_base<T>(std::forward<BasicJsonType>(j), tag, make_index_sequence<N> {});
}

template < typename BasicJsonType, typename ArrayType >
auto from_json(BasicJsonType&& j, identity_tag<ArrayType> tag)
-> decltype(from_json_inplace_array_impl(std::forward<BasicJsonType>(j), tag, priority_tag<0> {}))
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not how priority_tag is supposed to be used. You should have a single from_json overload and construct the tag with the highest value, like this:

template < typename BasicJsonType, typename ArrayType >
auto from_json(BasicJsonType&& j, ArrayType&& array)
-> decltype(from_json_inplace_array_impl(std::forward<BasicJsonType>(j),
                                         std::forward<ArrayType>(array), priority_tag<1> {}));

This gist shows quite nicely how to use it. Basically, it forces the overload resolution so you don't have to perform every SFINAE check in the world (like you had is_default_constructible and ! is_default_constructible)

Copy link
Contributor Author

@AnthonyVH AnthonyVH Jan 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Note that there's only 1 possible overload for from_json_inplace_array_impl(), hence 0 is the highest value in that case. Or am I missing something else?

The from_json_inplace_array_impl_base() is there because there's no index_sequence argument for from_json_inplace_array_impl(). I could add that and call make_index_sequence() from from_json(), but then any future from_json_inplace_array_impl() overload should have that as an argument as well, which didn't seem ideal. Hence the extra indirection.

I also looked into making this part of the from_json_array_impl() overloads, but there's SFINAE on its "entry-point" from_json() which prevents it from being called for non-default constructable types.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or am I missing something else?

No, it's me sorry. But in this case there is no need for priority_tag at all.

but there's SFINAE on its "entry-point" from_json() which prevents it from being called for non-default constructable types.

Just thought about it but non-default constructible types can very well use from_json(Json const&, T&) overloads, this happens when get_to is used for instance.

Therefore, the is_default_constructible check should just happen in get, and all functions that default-construct values before calling JSONSerializer<T>::from_json, no?

This would allow you to dispatch to from_json_array_impl and then use priority_tag.

It's been quite a while since I put my hands/eyes on the code, sorry for rusty review 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it's me sorry. But in this case there is no need for priority_tag at all.

True, I didn't have that there originally, but I gathered from one of your previous comments that you preferred this to make adding future overloads easier. I can remove it again if you prefer.

Therefore, the is_default_constructible check should just happen in get, and all functions that default-construct values before calling JSONSerializer::from_json, no?

Yes, that's true. Checking for default constructability in detail::from_json (e.g. the array version) shouldn't be necessary, since at that point you already have a reference to such an object, so it's already constructed.

Note that get() already has some checks using e.g. has_from_json and has_non_default_from_json. And those currently depend on default constructability of a type, since they depend the presence of certain signatures of from_json existing, and some of those in turn exist based on constructability of a type. Which as noted above actually isn't necessary...

So I guess the solution would be to get rid of constructability checks in from_json() and add an explicit std::is_default_constructible to get()?

I can take a look at it later this week. Although that might be overextending the scope of this PR. Or is that not an issue?

Copy link
Contributor

Choose a reason for hiding this comment

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

True, I didn't have that there originally, but I gathered from one of your previous comments that you preferred this to make adding future overloads easier.

When there is at least two overloads, yes 😄

Although that might be overextending the scope of this PR.

I think it's a part of it, then you adding a get overload with that deals with non-default constructible types that do not have has_non_default_from_json should do the trick (it would check if from_json(json, identity_tag<> is valid).

Icing on the cake would be to refactor all those get overloads to use priority_tag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When there is at least two overloads, yes 😄

Ok, I got ride of the redundant priority_tag and simplified some more code.

The LWG 2367 but mentioned below should also be fixed. Let's see what the regression says.

Also added some icing 😉.

{
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), tag, make_index_sequence<N> {});
return from_json_inplace_array_impl(std::forward<BasicJsonType>(j), tag, priority_tag<0> {});
}

template<typename BasicJsonType>
Expand Down Expand Up @@ -352,7 +358,6 @@ std::pair<A1, A2> from_json_pair_impl(BasicJsonType&& j, identity_tag<std::pair<
std::forward<BasicJsonType>(j).at(1).template get<A2>()};
}


template<typename BasicJsonType, typename A1, typename A2,
enable_if_t<std::is_default_constructible<std::pair<A1, A2>>::value, int> = 0>
void from_json_pair_impl(BasicJsonType && j, std::pair<A1, A2>& p, priority_tag<1> /*unused*/)
Expand Down
25 changes: 15 additions & 10 deletions single_include/nlohmann/json.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3746,25 +3746,31 @@ 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, identity_tag<Array> /*unused*/, index_sequence<Is...> /*unused*/)
template < typename T, typename BasicJsonType, typename ArrayType, std::size_t... Idx>
ArrayType from_json_inplace_array_impl_base(BasicJsonType&& j, identity_tag<ArrayType> /*unused*/,
index_sequence<Idx...> /*unused*/)
{
return { std::forward<BasicJsonType>(j).at(Is).template get<typename Array::value_type>()... };
return { std::forward<BasicJsonType>(j).at(Idx).template get<T>()... };
}

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, identity_tag<std::array<T, N>> tag)
-> decltype(j.template get<T>(),
from_json_array_impl(std::forward<BasicJsonType>(j), tag, make_index_sequence<N> {}))
template < typename BasicJsonType, typename T, std::size_t N >
auto from_json_inplace_array_impl(BasicJsonType&& j, identity_tag<std::array<T, N>> tag, priority_tag<0> /*unused*/)
-> decltype(from_json_inplace_array_impl_base<T>(std::forward<BasicJsonType>(j), tag, make_index_sequence<N> {}))
{
return from_json_inplace_array_impl_base<T>(std::forward<BasicJsonType>(j), tag, make_index_sequence<N> {});
}

template < typename BasicJsonType, typename ArrayType >
auto from_json(BasicJsonType&& j, identity_tag<ArrayType> tag)
-> decltype(from_json_inplace_array_impl(std::forward<BasicJsonType>(j), tag, priority_tag<0> {}))
{
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), tag, make_index_sequence<N> {});
return from_json_inplace_array_impl(std::forward<BasicJsonType>(j), tag, priority_tag<0> {});
}

template<typename BasicJsonType>
Expand Down Expand Up @@ -3849,7 +3855,6 @@ std::pair<A1, A2> from_json_pair_impl(BasicJsonType&& j, identity_tag<std::pair<
std::forward<BasicJsonType>(j).at(1).template get<A2>()};
}


template<typename BasicJsonType, typename A1, typename A2,
enable_if_t<std::is_default_constructible<std::pair<A1, A2>>::value, int> = 0>
void from_json_pair_impl(BasicJsonType && j, std::pair<A1, A2>& p, priority_tag<1> /*unused*/)
Expand Down
3 changes: 0 additions & 3 deletions test/src/unit-regression2.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -543,9 +543,6 @@ TEST_CASE("regression tests 2")
{
{
json j = { 3, 8 };
auto x = j.at(0).get<NonDefaultConstructible>();
CHECK(x.x == 3);

auto p = j.get<std::pair<NonDefaultConstructible, NonDefaultConstructible>>();
CHECK(p.first.x == 3);
CHECK(p.second.x == 8);
Expand Down