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 warning C4706 on Visual Studio 2017 #785

Merged
merged 5 commits into from
Oct 16, 2017
Merged

Conversation

jseward
Copy link
Contributor

@jseward jseward commented Oct 16, 2017

I'm pretty sure the logic is the same, just moves out the updating of variable keep. All tests pass at least.

This also fixes a bug in unit-udt which has a missing u8 causing crashes on windows (due to iterator assert)

@trilogy-service-ro
Copy link

Can one of the admins verify this patch?

Lots of tests have this warning.

Also moved out of for loop, doesn't need to be done every loop.
@nlohmann
Copy link
Owner

@trilogy-service-ro Could you please stop adding automated comments to every PR of this project? Of course, we have a look at each patch.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling af99090 on jseward:develop into d300a8e on nlohmann:develop.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling af99090 on jseward:develop into d300a8e on nlohmann:develop.

@@ -66,6 +66,19 @@ set_target_properties(catch_main PROPERTIES
)
target_include_directories(catch_main PRIVATE "thirdparty/catch")

# https://stackoverflow.com/questions/2368811/how-to-set-warning-level-in-cmake
if(MSVC)
# Force to always compile with W4
Copy link
Owner

Choose a reason for hiding this comment

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

Why do you want to force the /W4 flag?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With /W4 always enabled we can ensure no warnings will be emitted when adding this library to an existing project. It feels like a library such as this one should support /W4 with no issues. There are other warnings in unit tests even with /W3, but that would be for another PR. At least there is visibility now.

Copy link
Owner

Choose a reason for hiding this comment

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

Ok, thanks. I realized we're up to some 50 warnings now...

Copy link
Owner

Choose a reason for hiding this comment

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

I would really appreciate any help with MSVC regarding this warnings, as I have no experience here.

@nlohmann nlohmann self-assigned this Oct 16, 2017
@nlohmann nlohmann added this to the Release 3.0.0 milestone Oct 16, 2017
@nlohmann nlohmann added the platform: visual studio related to MSVC label Oct 16, 2017
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 nlohmann merged commit 7c8f0a4 into nlohmann:develop Oct 16, 2017
@nlohmann
Copy link
Owner

Thanks a lot!

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.

4 participants