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.

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

get<T> for types that are not default constructible #996

Closed
bradymadams opened this issue Mar 6, 2018 · 6 comments
Closed

get<T> for types that are not default constructible #996

bradymadams opened this issue Mar 6, 2018 · 6 comments
Labels
solution: proposed fix a fix for the issue has been proposed and waits for confirmation

Comments

@bradymadams
Copy link

bradymadams commented Mar 6, 2018

Currently the get<T> method requires that T is default constructible. Might it make sense to overload get so that it can take variadic arguments and construct the new object with a non-default constructor? For example - below is a minor variation of get. Any reason why this is a bad idea?

template<typename ValueTypeCV, typename ValueType = detail::uncvref_t<ValueTypeCV>,
         detail::enable_if_t <
             not std::is_same<basic_json_t, ValueType>::value and
             detail::has_from_json<basic_json_t, ValueType>::value and
             not detail::has_non_default_from_json<basic_json_t, ValueType>::value,
             int> = 0, typename ...Types>
ValueType get(Types... args) const noexcept(noexcept(
                                   JSONSerializer<ValueType>::from_json(std::declval<const basic_json_t&>(), std::declval<ValueType&>())))
{
    // we cannot static_assert on ValueTypeCV being non-const, because
    // there is support for get<const basic_json_t>(), which is why we
    // still need the uncvref
    static_assert(not std::is_reference<ValueTypeCV>::value,
                  "get() cannot be used with reference types, you might want to use get_ref()");

    ValueType ret(args...); // Allows non-default constructors to be used
    JSONSerializer<ValueType>::from_json(*this, ret);
    return ret;
}
@nlohmann
Copy link
Owner

nlohmann commented Mar 6, 2018

Do you have a concrete example for this?

@theodelrieu
Copy link
Contributor

There is a way to deserialize non-default constructible types, you can see this section for more details.

@theodelrieu
Copy link
Contributor

As for your suggestion, the problem is that from_json functions overwrite the ValueType object.

Even if your from_json functions take care of the already constructed value, it would have to handle the different calls to get, e.g.:

j.get<MyClass>(1, 2); // first constructor
j.get<MyClass>(); // default constructor
j.get<MyClass>("") // another constructor 

I think this can lead to lots of complex handling inside functions, so I would rather keep the conversion functions with lowest side-effects as possible.

@bradymadams
Copy link
Author

Just to make sure I understand - your concern is different constructors might leave the ValueType object in different states. It would get complicated for from_json to figure out the state of the object since there's no easy way of telling which constructor was used. Is that correct? That isn't something I thought of and it makes sense.

I can use the approach of specializing adl_serializer as you mentioned above. Thanks for the help!

@theodelrieu
Copy link
Contributor

Yes that's exactly it :)

@nlohmann nlohmann added the solution: proposed fix a fix for the issue has been proposed and waits for confirmation label Mar 6, 2018
@nlohmann
Copy link
Owner

nlohmann commented Mar 6, 2018

Thanks @theodelrieu !

@nlohmann nlohmann closed this as completed Mar 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
solution: proposed fix a fix for the issue has been proposed and waits for confirmation
Projects
None yet
Development

No branches or pull requests

3 participants