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 #16

Open
wants to merge 4 commits into
base: feature/optional
Choose a base branch
from
Open

fix problems in PR2117 #16

wants to merge 4 commits into from

Conversation

dota17
Copy link
Owner

@dota17 dota17 commented Jun 22, 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 nlohmann/json#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)
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

@coveralls
Copy link

coveralls commented Jun 22, 2020

Coverage Status

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

@dota17 dota17 changed the title fix compiler error C2440 in VS fix problems for PR-2117 Jun 24, 2020
@dota17 dota17 changed the title fix problems for PR-2117 fix problems for PR2117 Jun 24, 2020
@dota17 dota17 changed the title fix problems for PR2117 fix problems in PR2117 Jun 24, 2020
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.

2 participants