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

Fix problems in PR2117 #2213

Closed
wants to merge 4 commits into from
Closed

Conversation

dota17
Copy link
Contributor

@dota17 dota17 commented Jun 24, 2020

This PR would fix some problems in PR#2117.

  1. It would solve the AppVeyor compile error. Probably it is a bug in the VS compiler.

  2. As Add conversion from/to std::optional #2117 (comment) said, I found some problems.

2.a. I used these code to detect but just found JSON_HAS_CPP_17 = False in all travis jobs.

#ifdef JSON_HAS_CPP_17
    std::cout << "JSON_HAS_CPP_17 = True" << std::endl;
#else
	std::cout << "JSON_HAS_CPP_17 = Flase" << std::endl;
#endif

Thus, All travis jobs didn't run the testcase - TEST_CASE("std::optional"), and those all code about optional were not compiled.

I add a new travis task for c++17.

2.b. I also found the actual behavior, which would also happen in linux:

CHECK(std::optional<std::string>(j_string) == opt_string);  // call from_json(const BasicJsonType& j, ConstructibleStringType& s) in WIN, call from_json(const BasicJsonType& j, typename BasicJsonType::string_t& s) in Linux
CHECK(std::optional<bool>(j_bool) == opt_bool); // call from_json(const BasicJsonType& j, typename BasicJsonType::boolean_t& b) 
CHECK(std::optional<int>(j_number) == opt_int); // call from_json(const BasicJsonType& j, ArithmeticType& val)

And the testcase SECTION("null") didn't call the expected method either and raise an excepiton - "[json.exception.type_error.302] type must be object, but is null".

I agree with this view:

The reason is the template optional(U&&) constructor template, which is supposed to activate when T (int) is constructible from U and U is neither std::in_place_t nor optional, and direct-initialize T from it. And so it does, stamping out optional(foo&).

Thus, they would not call the method from_json(basi_json, std::option<T>) never. This is why an exception is raised in SECTION("null").

Related Problem:
Different results in Clang and GCC when casting to std::optional
Why is a cast operator to std::optional ignored?
specification of std::optional constructor regarding cast operators

@dota17 dota17 requested a review from nlohmann as a code owner June 24, 2020 07:39
@coveralls
Copy link

coveralls commented Jun 24, 2020

Coverage Status

Coverage remained the same at 100.0% when pulling 7df758f on dota17:fix-C2440 into b61d563 on nlohmann:feature/optional.

.travis.yml Outdated Show resolved Hide resolved
@@ -1710,7 +1710,8 @@ TEST_CASE("std::optional")
std::optional<std::string> opt_null;

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

Choose a reason for hiding this comment

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

Why is null not converted to a nullopt here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See 2.b in my comments.

I agree with this view:

The reason is the template optional(U&&) constructor template, which is supposed to activate when T (int) is constructible from U and U is neither std::in_place_t nor optional, and direct-initialize T from it. And so it does, stamping out optional(foo&).

j_null is basic_json, and there is a way that basic_json can be converted to string by calling void from_json(const BasicJsonType& j, typename BasicJsonType::string_t& s)

Copy link
Owner

Choose a reason for hiding this comment

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

This may be a technical explanation, but as I user, when I ask to translate a JSON value to a std::optional<std::string>, then I expect this behavior:

  • string values are stored as value,
  • null is stored as std::nullopt, and
  • all other values throw

At least this is what the code

#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

tries to accomplish.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants