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

Add defines to replace magic numbers in inflate fast #302

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add defines to replace magic numbers in inflate fast #302

wants to merge 1 commit into from

Conversation

ProgramMax
Copy link

@nigeltao has a really great patch over at #292
In that patch, there are two #defines that can be moved out into a separate pull request to make a smaller, easier-to-accept pull request. Nigel granted me permission to make a separate pull request.

The great benefit of this patch is it replaces magic numbers with a clear name. This makes the code easier to understand & maintain.

@ProgramMax
Copy link
Author

@Adenilson could you take a look at this as well and tell me if there are ways I could improve it? Thanks!

@Adenilson
Copy link

I'm assuming that 'make test' is working fine with this change? It should as this is just a macro substitution.

It may make sense to update the description of the algorithm, where it is explicitly described that:
"Distances are limited to 32K bytes, and lengths are limited to 258 bytes."

I'm referring to:
https://github.com/madler/zlib/blob/master/doc/algorithm.txt#L8

Maybe change it to:
"Distances are limited to 32K bytes, and lengths are limited to 258 bytes (INFLATE_FAST_MIN_LEFT)."

@ProgramMax
Copy link
Author

1.) Yep, './configure' followed by 'make test' passed.

2.) It is an interesting idea to update the algorithm docs. If the docs reference ONLY the define then they won't get out of sync. It also clues the reader to look for that define. Using both number & define risks getting out of sinc but provides that clue.

However, I don't think the algorithm docs elsewhere reference a define. For example, the doc mentions 15 bits instead of MAX_WBITS. Maybe that just means there are more magic numbers we can replace with defines if we look. :D

I'm okay with which ever option. If you think adding the define is most helpful I'll do that. But I wanted to lay out all the options and think them through here.

Let me know if you think the added define is best. I certainly see value in it. If you think it's best I'll add it.

@nigeltao
Copy link

Well, the file is called algorithm.txt, but on my reading, the "lengths are limited to 258 bytes" fragment describes a fundamental limitation of the wire format, not of any one particular implementation.

Different implementations can use a higher INFLATE_FAST_MIN_LEFT value, if the additional assumption enables certain optimization techniques, but the format itself is still limited to 258.

Therefore, my opinion is not to change algorithm.txt, but I'll leave the decision to others.

@nigeltao
Copy link

Additionally, that fragment occurs in the "Compression algorithm (deflate)" section of algorithm.txt, and note that it's deflate, not inflate, but the INFLATE_FAST_MIN_LEFT macro only affects the inflate implementation.

@ProgramMax
Copy link
Author

That makes a lot of sense to me.
As soon as we land a patch that alters INFLATE_FAST_MIN_LEFT, we would be making the documentation less clear.
You've sold me. :)

@Adenilson
Copy link

I agree with Nigel's input, it makes sense to keep the current patch as it is.
o/

hzhuang1 pushed a commit to Linaro/warpdrive-zlib that referenced this pull request Jul 31, 2019
Fixed arithmetic overflow warnings in MSVC.
Fixed uint64_t to uint32_t casting warning.
Added assert to check if bits is greater than 32 before cast.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants