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.

Sign up for GitHub

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

Jump to bottom

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
nlohmann
nlohmann requested changes Oct 17, 2017
View reviewed changes
test/src/unit-readme.cpp

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

test/CMakeLists.txt Outdated
@@ -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!

nlohmann
nlohmann approved these changes Oct 18, 2017
View reviewed changes
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
Reviewers

@nlohmann nlohmann nlohmann approved these changes

Assignees

@nlohmann nlohmann

Labels
platform: visual studio related to MSVC
Projects
None yet
Milestone
Release 3.0.0
Development

Successfully merging this pull request may close these issues.
5 participants
@jseward @trilogy-service-ro @coveralls @nlohmann @gregmarr