Skip to content
This repository has been archived by the owner on Mar 1, 2024. It is now read-only.

Address C4244 warnings in MSVC build #41

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

GrabYourPitchforks
Copy link

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 conventions already used throughout the project.

The changes to deflate.c and trees.c are the same as I submitted to madler/zlib#852. See that PR for discussion. If you're going to rebase on madler/zlib at some future point, then perhaps you might opt to leave these changes out of this repo, instead pulling in the changes the next time you take a rebase.

The change to deflate_quick.c (line 139) appears correct because at the start of the method, the value argument fits cleanly into a 16-bit integer (confirmed by checking the three call sites), length is no greater than 16, and bi_valid is no greater than 16. This means that lines 128 - 130 won't result in a loss of fidelity. By the time line 137 is reached, out will contain at most 7 unpended / unflushed bits, so the narrowing conversion to a 16-bit backing field (line 139) is lossless.

I cannot convince myself of the correctness of deflate_quick.c (line 227) because I cannot easily follow the logic backward. I assume the narrowing conversion here is intentional, but I would feel more comfortable if a project maintainer would validate this assumption.

The change to slide_sse.c appears correct because the value of w_size is limited to 1 << 15. See below.

zlib/deflate.c

Lines 306 to 310 in d9c2508

if (memLevel < 1 || memLevel > MAX_MEM_LEVEL || method != Z_DEFLATED ||
windowBits < 8 || windowBits > 15 || level < 0 || level > 9 ||
strategy < 0 || strategy > Z_FIXED || (windowBits == 8 && wrap != 1)) {
return Z_STREAM_ERROR;
}

zlib/deflate.c

Lines 325 to 326 in d9c2508

s->w_bits = (uInt)windowBits;
s->w_size = 1 << s->w_bits;

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

Successfully merging this pull request may close these issues.

2 participants