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 conversion from/to std::optional #2117

Closed
wants to merge 16 commits into from
Closed

Add conversion from/to std::optional #2117

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
df30a0e
:construction: conversions for std::optional
nlohmann Nov 22, 2019
1359c56
:checkered_flag: fix <optional> inclusion
nlohmann Nov 23, 2019
713038f
:green_heart: overwork tests
nlohmann Nov 23, 2019
1b846c9
Use JSON_HAS_CPP_17 only after it has been defined
remedi Dec 17, 2019
a2fea87
Merge pull request #1872 from remedi/feature/optional
nlohmann Dec 18, 2019
6b31375
:alembic: set CXXFLAGS
nlohmann Dec 19, 2019
71c80cc
Merge branches 'develop' and 'feature/optional' of https://github.com…
nlohmann May 16, 2020
007c6c4
:white_check_mark: update tests
nlohmann May 16, 2020
f25bf31
:checkered_flag: include right <optional> header
nlohmann May 17, 2020
b61d563
Merge branches 'develop' and 'feature/optional' of https://github.com…
nlohmann May 20, 2020
fedf82c
Merge branch 'develop' of https://github.com/nlohmann/json into featu…
nlohmann Aug 7, 2020
5e55158
Merge branch 'develop' of https://github.com/nlohmann/json into featu…
nlohmann Dec 21, 2020
ebb63bd
Merge branch 'develop' of https://github.com/nlohmann/json into featu…
nlohmann Dec 30, 2020
f1913fe
:recycle: do not include experimental headers
nlohmann Jan 3, 2021
384442e
:fire: remove useless line
nlohmann Jan 3, 2021
7ffd3ae
Merge branch 'develop' of https://github.com/nlohmann/json into featu…
nlohmann Jan 3, 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
15 changes: 15 additions & 0 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 @@ -32,6 +32,21 @@ void from_json(const BasicJsonType& j, typename std::nullptr_t& n)
n = nullptr;
}

#ifdef JSON_HAS_CPP_17
template<typename BasicJsonType, typename T>
void from_json(const BasicJsonType& j, std::optional<T>& opt)
{
if (j.is_null())
{
opt = std::nullopt;
}
else
{
opt = j.template get<T>();
}
}
#endif

// overloads for basic_json template parameters
template < typename BasicJsonType, typename ArithmeticType,
enable_if_t < std::is_arithmetic<ArithmeticType>::value&&
Expand Down
16 changes: 16 additions & 0 deletions include/nlohmann/detail/conversions/to_json.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,22 @@ struct external_constructor<value_t::object>
// to_json //
/////////////

#ifdef JSON_HAS_CPP_17
template<typename BasicJsonType, typename T,
enable_if_t<std::is_constructible<BasicJsonType, T>::value, int> = 0>
void to_json(BasicJsonType& j, const std::optional<T>& opt)
{
if (opt.has_value())
{
j = *opt;
}
else
{
j = nullptr;
}
}
#endif

template<typename BasicJsonType, typename T,
enable_if_t<std::is_same<T, typename BasicJsonType::boolean_t>::value, int> = 0>
void to_json(BasicJsonType& j, T b) noexcept
Expand Down
4 changes: 4 additions & 0 deletions include/nlohmann/detail/macro_scope.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,10 @@
#define JSON_HAS_CPP_14
#endif

#ifdef JSON_HAS_CPP_17
#include <optional>
#endif

// disable float-equal warnings on GCC/clang
#if defined(__clang__) || defined(__GNUC__) || defined(__GNUG__)
#pragma GCC diagnostic push
Expand Down
35 changes: 35 additions & 0 deletions single_include/nlohmann/json.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2108,6 +2108,10 @@ JSON_HEDLEY_DIAGNOSTIC_POP
#define JSON_HAS_CPP_14
#endif

#ifdef JSON_HAS_CPP_17
#include <optional>
#endif

// disable float-equal warnings on GCC/clang
#if defined(__clang__) || defined(__GNUC__) || defined(__GNUG__)
#pragma GCC diagnostic push
Expand Down Expand Up @@ -3518,6 +3522,21 @@ void from_json(const BasicJsonType& j, typename std::nullptr_t& n)
n = nullptr;
}

#ifdef JSON_HAS_CPP_17
template<typename BasicJsonType, typename T>
void from_json(const BasicJsonType& j, std::optional<T>& opt)
{
if (j.is_null())
{
opt = std::nullopt;
}
else
{
opt = j.template get<T>();
}
}
#endif

// overloads for basic_json template parameters
template < typename BasicJsonType, typename ArithmeticType,
enable_if_t < std::is_arithmetic<ArithmeticType>::value&&
Expand Down Expand Up @@ -4294,6 +4313,22 @@ struct external_constructor<value_t::object>
// to_json //
/////////////

#ifdef JSON_HAS_CPP_17
template<typename BasicJsonType, typename T,
enable_if_t<std::is_constructible<BasicJsonType, T>::value, int> = 0>
void to_json(BasicJsonType& j, const std::optional<T>& opt)
{
if (opt.has_value())
{
j = *opt;
}
else
{
j = nullptr;
}
}
#endif

template<typename BasicJsonType, typename T,
enable_if_t<std::is_same<T, typename BasicJsonType::boolean_t>::value, int> = 0>
void to_json(BasicJsonType& j, T b) noexcept
Expand Down
59 changes: 59 additions & 0 deletions test/src/unit-conversions.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1703,6 +1703,65 @@ TEST_CASE("JSON to enum mapping")
}
}

#ifdef JSON_HAS_CPP_17
TEST_CASE("std::optional")
{
SECTION("null")
{
json j_null;
std::optional<std::string> opt_null;

CHECK(json(opt_null) == j_null);
CHECK(std::optional<std::string>(j_null) == std::nullopt);
Copy link
Contributor

Choose a reason for hiding this comment

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

Initialization of optional<T> from empty json should be considered as a type_error:

Suggested change
CHECK(std::optional<std::string>(j_null) == std::nullopt);
CHECK_THROWS_AS(std::optional<std::string>(j_null), json::type_error&);

The reason is as follows. If json is implicitly convertible to T, then initialization of optional<T> from json almost necessarily goes through the "forwarding" constructor:

template <class U> optional(U&& value); // unrelated details omitted

This constructor provides the best parameter/argument match by value category, constness and a type. Its parameter binds directly to json in any form. The only "weakness" of the forwarding constructor is that it is a template. A conversion function that would be considered as a better candidate for overload resolution must be non-templated, but this is obviously unacceptable.

The forwarding constructor converts json to T (initializes the latter from the former) and stores the result, so the constructed optional<T> cannot be empty. Such a conversion from empty json to T is a type error (even if T itself is optional<X>, by induction).

The non-standard overload resolution performed by GCC and Clang when enabling C++17 does not change the outcome. The only way to achieve the "natural" behavior is to tune constructors of std::optional<T>, which is beyond the scope of the project.

Copy link
Contributor

@karzhenkov karzhenkov Jan 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

There is also the usual way to explicitly convert json to optional<T>. The following will work:

Suggested change
CHECK(std::optional<std::string>(j_null) == std::nullopt);
CHECK(j_null.get<std::optional<std::string>>() == std::nullopt);

Copy link
Contributor

@karzhenkov karzhenkov Jan 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Essentially, there is nothing new here. @dota17 proposed the same in #2213.
However, this approach (treat such initialization as type_error) introduces some inconsistency:

std::optional<int> opt1 = json().get<std::optional<int>>(); // ok: std::nullopt
std::optional<int> opt2 = std::optional<int>(json());       // throws type_error
std::optional<int> opt3 = json();                           // throws type_error

Perhaps it makes sense to throw type_error in the first case too?
The "natural" conversion could be explicitly implemented by dedicated member function:

template <typename T> std::optional<T> get_optional() const;

Copy link
Contributor

Choose a reason for hiding this comment

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

Moreover, as it turns out, this dedicated member function is the only meaningful thing nlohmann::basic_json can provide to support std::optional. Overloads of get<std::optional<T>> should probably be disabled to prevent confusion.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I think the behavior is counter-intuitive. JSON's null is usually considered as "unset" value (in contrast to an empty value). Therefore, I would expect null to convert to std::nullopt of any optional type.

}

SECTION("string")
{
json j_string = "string";
std::optional<std::string> opt_string = "string";

CHECK(json(opt_string) == j_string);
CHECK(std::optional<std::string>(j_string) == opt_string);
}

SECTION("bool")
{
json j_bool = true;
std::optional<bool> opt_bool = true;

CHECK(json(opt_bool) == j_bool);
CHECK(std::optional<bool>(j_bool) == opt_bool);
}

SECTION("number")
{
json j_number = 1;
std::optional<int> opt_int = 1;

CHECK(json(opt_int) == j_number);
CHECK(std::optional<int>(j_number) == opt_int);
}

SECTION("array")
{
json j_array = {1, 2, nullptr};
std::vector<std::optional<int>> opt_array = {{1, 2, std::nullopt}};

CHECK(json(opt_array) == j_array);
CHECK(std::vector<std::optional<int>>(j_array) == opt_array);
Copy link
Contributor

Choose a reason for hiding this comment

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

Direct initialization of vector<...> is not suitable here:

Suggested change
CHECK(std::vector<std::optional<int>>(j_array) == opt_array);
CHECK(j_array.get<std::vector<std::optional<int>>>() == opt_array);

Such kind of initialization of vector<...> has not been tested even with simple item types and actually leads to a compile error for the reason considered earlier. For example, the following will not compile unless using GCC or Clang with C++17 enabled (see https://godbolt.org/z/oeooPG):

json j({1, 2, 3});
std::vector<int> v1 = j;         // Copy initialization: ok
auto v2 = std::vector<int>(j);   // Direct initialization: compile error

}

SECTION("object")
{
json j_object = {{"one", 1}, {"two", 2}, {"zero", nullptr}};
std::map<std::string, std::optional<int>> opt_object {{"one", 1}, {"two", 2}, {"zero", std::nullopt}};

CHECK(json(opt_object) == j_object);
CHECK(std::map<std::string, std::optional<int>>(j_object) == opt_object);
Copy link
Contributor

Choose a reason for hiding this comment

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

Direct initialization of map<...> has the same trouble as with vector<...> above:

Suggested change
CHECK(std::map<std::string, std::optional<int>>(j_object) == opt_object);
CHECK(j_object.get<std::map<std::string, std::optional<int>>>() == opt_object);

}
}
#endif

#ifdef JSON_HAS_CPP_17
#undef JSON_HAS_CPP_17
#endif
Expand Down