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 msvc warnings #1846

Merged
merged 2 commits into from
Nov 19, 2019
Merged

Fix msvc warnings #1846

merged 2 commits into from
Nov 19, 2019

Conversation

MBalszun
Copy link
Contributor

@MBalszun MBalszun commented Nov 19, 2019

This PR:

  1. Changes the accumulation variable for size computations in binar_writer from unsiged long to size_t [*]
  2. Remove two warning suppressions in msvc for warnings that seem to no longer appear anyway (waiting for CI results to verify [EDIT: Can confirm no new warnings appeared in CI])

*) In the x64 platofrm on windows std::size_t (64 bit unsigned integer) is not the same as an unsigned long (32bit unsigned integer on all platforms targets). This leads to a (correct) warning about possible loss of data. I'm not sure if there is an actual situation, where this variable can overflow, but the warning itself needs to be fixed anyway and I believe the intent was to use a variable of type std::size_t (which is the same as unsigned long on almost all other platforms) anyway.

[EDIT: ] If preferred, I can split up the PR into one for 1. and 2.


Pull request checklist

Read the Contribution Guidelines for detailed information.

  • Changes are described in the pull request, or an existing issue is referenced.
  • The test suite compiles and runs without error.
  • Code coverage is 100%. Test cases can be added by editing the test suite.
  • The source code is amalgamated; that is, after making changes to the sources in the include/nlohmann directory, run make amalgamate to create the single-header file single_include/nlohmann/json.hpp. The whole process is described here.

@MBalszun MBalszun changed the title Fix warnings Fix "possible loss of data" warnings Nov 19, 2019
@MBalszun MBalszun changed the title Fix "possible loss of data" warnings Fix msvc warnings Nov 19, 2019
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 25e0175 on Mike-Bal:fix_warnings into e7b3b40 on nlohmann:develop.

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 self-assigned this Nov 19, 2019
@nlohmann nlohmann added this to the Release 3.7.4 milestone Nov 19, 2019
@nlohmann nlohmann merged commit b2bec94 into nlohmann:develop Nov 19, 2019
@nlohmann
Copy link
Owner

Thanks!

@nlohmann
Copy link
Owner


🔖 Release item

This issue/PR will be part of the next release of the library. This template helps preparing the release notes.

Type

  • ✨ New Feature
  • 🐛 Bug Fix
  • ⚡️ Improvement
  • 🔨 Further Change
  • 🔥 Deprecated function

Description

  • An MSVC warning has been fixed which no longer needs to be suppressed when compiling the test suite. Fix msvc warnings #1846

@MBalszun
Copy link
Contributor Author

Thanks for the quick merge.

Just a comment on the release item description: The warning that was fixed and the warnings that got suppressed where independent of each other i.e. the warnings about possible loss of data popped up even with the suppression still active and removing the suppression would not pop up any new warnings either way. As the suppressions only existed in unit tests, I don't think they need to be mentioned in the release item description.

Sorry for putting them into one PR.

@nlohmann
Copy link
Owner

I see. What would you propose as alternative items for the release notes?

@MBalszun
Copy link
Contributor Author

Would have to look at your previous notes for reference (i.e. not sure, what the typical level of detail is).

Probably something like either "Fix warning about possible loss of data on windows x64" or "Fix integer type mismatch in binary_writer size computation on windows x64"

@MBalszun MBalszun deleted the fix_warnings branch November 21, 2019 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants