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

Remove unneeded casts #734

Closed
wants to merge 1 commit into from
Closed

Remove unneeded casts #734

wants to merge 1 commit into from

Conversation

AZero13
Copy link

@AZero13 AZero13 commented Oct 28, 2022

These casts are redundant

These casts are redundant
@AraHaan
Copy link
Contributor

AraHaan commented Oct 28, 2022

I do not know if the changes here will compile when my PR #732 gets merged (with ARM and ARM64 targets for zlib for Windows, Linux, and MacOS (except ARM and x86 for Mac))

@AraHaan
Copy link
Contributor

AraHaan commented Apr 19, 2023

After thinking about this, I think the casts are actually needed as it could change how the code actually works and could produce bugs later that breaks the entire thing, when I tried a change similar to this with my 100% .NET implementation of zlib (without the use of the native library) I noticed that then I accidentally broke it to where the zlib header and footers are good and created, but then it broke it to where it basically just copies the original data to the resulting "compressed data" buffer without actually compressing it, despite the zlib header and footer saying that it is compressed.

@madler
Copy link
Owner

madler commented Apr 19, 2023

The commit is not all cast changes. In any case, I see no point in making any of these changes. Code is not just for the compiler. If it were, we could just obfuscate it all. Code is also literature, a communication from one human to another. A "redundant" cast may be entirely intended as a signal of intent.

@madler madler closed this Apr 19, 2023
@AZero13 AZero13 deleted the update branch May 2, 2023 15:04
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