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

Use JSON_HAS_CPP_17 only after it has been defined #1872

Merged
merged 1 commit into from
Dec 18, 2019

Conversation

remedi
Copy link

@remedi remedi commented Dec 17, 2019

Fix for Visual Studio "error C2039: 'optional': is not a member of 'std'" #1749

@remedi
Copy link
Author

remedi commented Dec 17, 2019

I seem to have messed up the amalgamation. I will try to figure out how the files are combined so that the JSON_HAS_CPP_17 is used only after language standard detection in single_include json.hpp.

@remedi
Copy link
Author

remedi commented Dec 17, 2019

Okay now the amalgamation issue is fixed. I am not sure if this a proper place for including, but I moved the #include to macro_scope.hpp just after the language standard detection.

Copy link
Owner

@nlohmann nlohmann left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Good catch!

@nlohmann
Copy link
Owner

Let's wait for the CI to become green.

@remedi
Copy link
Author

remedi commented Dec 17, 2019

Sorry for re-triggering the CI. Fixed committer email.

@coveralls
Copy link

coveralls commented Dec 17, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling 1b846c9 on remedi:feature/optional into 713038f on nlohmann:feature/optional.

@nlohmann
Copy link
Owner

There still seems to be an issue in the latest AppVeyor builds.

@remedi
Copy link
Author

remedi commented Dec 18, 2019

Yes it is the conversion issue that I mentioned in #1749.

However, now there is another error in unit-conversions.cpp lines 1624 and 1633 where there is
no suitable conversion from nlohmann::json to std::vector containing std::optional (1624) or
std::map containing std::optional (1633). I am not familiar with the code base so I am unsure
where exactly the conversion should happen.

I can try looking into this myself today, but I am not sure if I can find the correct place. What I think is missing is implicit conversion from nlohmann::json to a type that can be passed as an argument to std::vector<std::optional<int>> and std::map<std::string, std::optional<int>> constructors.

The compiler errors are:
C:\projects\json\test\src\unit-conversions.cpp(1624): error C2440: '<function-style-cast>': cannot convert from 'nlohmann::json' to 'std::vector<std::optional<int>,std::allocator<_Ty>>'
and
C:\projects\json\test\src\unit-conversions.cpp(1633): error C2440: '<function-style-cast>': cannot convert from 'nlohmann::json' to 'std::map<std::string,std::optional<int>,std::less<_Kty>,std::allocator<std::pair<const _Kty,_Ty>>>'

I was kind of hoping that you would immediately know what would be required.

@nlohmann
Copy link
Owner

I see. I did not have a look at the error message, but just hoped that the PR would fix them ;-)

I have no idea what the issue is. I am not expert with MSVC, and since GCC and Clang compile the code, it seems OK. However, I think merging your changes into the feature branch is a good idea anyway since it fixes the issue with the macro definition.

@nlohmann nlohmann merged commit a2fea87 into nlohmann:feature/optional Dec 18, 2019
@remedi
Copy link
Author

remedi commented Dec 18, 2019

Thanks for merging the PR. If you mean that Travis compiles the code on GCC and Clang you would be mistaken since looking at the build logs only 2 test cases (270 assertions) are being run for test-conversions.exe for all builds regardless of the compiler or options used.

When JSON_HAS_CPP_17 is defined there should be 3 test cases (298 assertions). I think there is something wrong with the language standard detection for GCC and Clang so JSON_HAS_CPP_17 is always undefined therefore the problematic unit test code does not seem to be compiled on GCC or Clang at all.

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