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 Visual Studio 2017 warnings #788

Merged
merged 2 commits into from
Oct 18, 2017
Merged

Fix Visual Studio 2017 warnings #788

merged 2 commits into from
Oct 18, 2017

Conversation

jseward
Copy link
Contributor

@jseward jseward commented Oct 17, 2017

With this PR there should now be 0 warnings in Visual Studio 2017 /W4 when compiling all tests.

@trilogy-service-ro
Copy link

Can one of the admins verify this patch?

@nlohmann nlohmann added the platform: visual studio related to MSVC label Oct 17, 2017

#if defined(_MSC_VER)
#pragma warning (pop)
#endif
Copy link
Owner

Choose a reason for hiding this comment

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

Could you please add a newline here?

@@ -75,8 +75,11 @@ if(MSVC)
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /W4")
endif()

# Disable warning C4389: '==': signed/unsigned mismatch
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /wd4389")
# warning C4389: '==': signed/unsigned mismatch
Copy link
Owner

Choose a reason for hiding this comment

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

Could you please add to the comment that these warnings are disabled?

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 917d9d8 on jseward:develop into 7c8f0a4 on nlohmann:develop.

@jseward
Copy link
Contributor Author

jseward commented Oct 18, 2017

Done and done!

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.

@nlohmann
Copy link
Owner

In MSVC 2015, there is still one warning left, which does not seem to occur in MSVC 2017:

json.hpp(7950): warning C4702: unreachable code [C:\projects\json\test\test-allocator.vcxproj]

See https://ci.appveyor.com/project/nlohmann/json/build/2315/job/f15evu7h4lpcxog8

This is really strange. Do you have any ideas about this?

@nlohmann nlohmann self-assigned this Oct 18, 2017
@nlohmann nlohmann added this to the Release 3.0.0 milestone Oct 18, 2017
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling a99fcb4 on jseward:develop into 7c8f0a4 on nlohmann:develop.

@gregmarr
Copy link
Contributor

The allocator used in this test always throws, so that code is indeed unreachable. It probably takes some really aggressive inlining to determine that. It could be that VS2017 isn't doing as much inlining as VS2015 in that particular area. It could also be that the unreachable code warning has been modified to allow code after an unconditional throw.

@nlohmann
Copy link
Owner

@gregmarr Thanks, this makes sense.

@nlohmann nlohmann merged commit 2e281ba into nlohmann:develop Oct 18, 2017
@nlohmann
Copy link
Owner

Thanks @jseward !

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

Successfully merging this pull request may close these issues.

5 participants