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

Possible incorrect _MSC_VER reference #2112

Closed
ua-code-dragon opened this issue May 15, 2020 · 7 comments · Fixed by #2137
Closed

Possible incorrect _MSC_VER reference #2112

ua-code-dragon opened this issue May 15, 2020 · 7 comments · Fixed by #2137
Assignees
Labels
platform: visual studio related to MSVC release item: 🔨 further change solution: proposed fix a fix for the issue has been proposed and waits for confirmation
Milestone

Comments

@ua-code-dragon
Copy link

#if defined(JSON_HAS_CPP_17) && (defined(__GNUC__) || (defined(_MSC_VER) and _MSC_VER <= 1914))

This condition seems to check "Visual Studio 2017 or later", so there should be >= 1914, not <=

@nlohmann
Copy link
Owner

This comparison was introduced in #1028 and last changed in #1496. In #1028 (comment), I think the reason is described why it should be <=:

So the bug preventing enabling this feature on msvc has been fixed: https://developercommunity.visualstudio.com/content/problem/243183/stdstring-should-not-have-a-operator-that-takes-a.html

It should be out in 15.8 (msvc 19.15?), so in 19.14 and older it is disabled, as discussed.

@nlohmann nlohmann added the solution: invalid the issue is not related to the library label May 16, 2020
@ua-code-dragon
Copy link
Author

Maybe I'm wrong, but std::string_view is only available from C++17. For example, Visual Studio 2015 has _MSC_VER=1900, so this #if becomes true and I got an error on unknown string_view.

@nlohmann
Copy link
Owner

Good point. I'm not using MSVC myself, but I may be able to check via AppVeyor.

@nlohmann nlohmann added platform: visual studio related to MSVC state: please discuss please discuss the issue or vote for your favorite option and removed solution: invalid the issue is not related to the library labels May 16, 2020
@ua-code-dragon
Copy link
Author

According to this the N4562 feature set with string_view has been added in VS 2017 15.0
According to this there was _MSC_VER=1910
Maybe the mentioned condition should be ... || (defined(_MSC_VER) and _MSC_VER >= 1910 and _MSC_VER <= 1914))

@garethsb
Copy link
Contributor

Doesn't the check for JSON_HAS_CPP_17 already deal with the beginning of the range?

@dota17
Copy link
Contributor

dota17 commented May 25, 2020

Doesn't the check for JSON_HAS_CPP_17 already deal with the beginning of the range?

As I can see, the check for JSON_HAS_CPP_17 and _MSC_VER is a parallel relationship(||),
so I agree with @ua-code-dragon in #2112 (comment).

@dota17 dota17 mentioned this issue May 26, 2020
@nlohmann nlohmann linked a pull request May 27, 2020 that will close this issue
@nlohmann nlohmann added the solution: proposed fix a fix for the issue has been proposed and waits for confirmation label May 27, 2020
@nlohmann nlohmann added release item: 🔨 further change and removed state: please discuss please discuss the issue or vote for your favorite option labels May 27, 2020
@nlohmann nlohmann self-assigned this May 27, 2020
@nlohmann nlohmann added this to the Release 3.8.0 milestone May 27, 2020
@nlohmann
Copy link
Owner

Thanks everyone for the discussion!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform: visual studio related to MSVC release item: 🔨 further change solution: proposed fix a fix for the issue has been proposed and waits for confirmation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants