madler / zlib

A massively spiffy yet delicately unobtrusive compression library.
http://zlib.net/
Other
5.58k stars 2.43k forks source link

Address C4244 warnings in MSVC build #852

Closed GrabYourPitchforks closed 7 months ago

GrabYourPitchforks commented 1 year ago

Allows the MSVC build to compile clean when the C4244 warning code (checking for potentially lossy implicit integer narrowing casts) is enabled. I've tried to follow the same coding convention as was used in https://github.com/madler/zlib/commit/3df842426b53522e232da7aa06d5ef10eeb5ec4a.

An analysis our review team performed to convince ourselves of the correctness of the changes...

The change to deflate.c is legal because 'len' has an upper bound of MAX_STORED, which means it fits cleanly into a 16-bit integer. So writing out 2x 8-bit values will not result in data loss.

The change to trees.c is legal because within this loop, 'count' is intended to have an upper bound of 138, with the target assignment only executing if 'count' is bounded by 4. Neither the 'count' local in isolation nor the addition that's part of the target line is expected to result in integer overflow. But even if it did, that's a matter for a different warning code and doesn't impact the correctness of the narrowing cast being considered here.

AraHaan commented 11 months ago

Interesting, so this is affecting dotnet/runtime? I wonder how my vc17 changes affect dotnet/runtime by chance.

GrabYourPitchforks commented 11 months ago

Interesting, so this is affecting dotnet/runtime? I wonder how my vc17 changes affect dotnet/runtime by chance.

It affects our ability to enable more C++ warnings as errors within our build. We've already made these changes locally so that we can re-enable the warning codes, but I'm sending them back upstream in case the project maintainer wants to take them. As far as I can tell there's no behavioral change.

AraHaan commented 10 months ago

Alright thanks, I was wondering if it would affect dotnet/runtime in a good way as well.

Neustradamus commented 7 months ago

@madler: Can you look this PR?

Linked to:

madler commented 7 months ago

Incorporated.