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

feat: impl From<Flush> to MZFlush #462

Merged
merged 5 commits into from
Feb 19, 2025

Conversation

CosminPerRam
Copy link
Contributor

@CosminPerRam CosminPerRam commented Feb 13, 2025

We would never hit an error in building a MZFlush, because all values of FlushDecompress and FlushCompress (as i32) map to MZFlush.
Implementing From to it is simpler and better as we get rid of the redundant .unwrap()s, casts to i32 and can see directly the translation.

For more context for the mapping done here, see MZFlush::new.

@CosminPerRam CosminPerRam changed the title feat: impl From<Flush> to MZFlush to avoid redundant unwrap usage feat: impl From<Flush> to MZFlush Feb 13, 2025
@CosminPerRam
Copy link
Contributor Author

CosminPerRam commented Feb 14, 2025

I also reordered FlushCompress variants to be more in line with their integer values (before it was 0, 2, 1, 3, 4 now its 0, 1, 2, 3, 4).

@CosminPerRam
Copy link
Contributor Author

Also found a spelling mistake and a missing ).

Copy link
Member

@Byron Byron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for your continued contributions, they are much appreciated!

I also like the small, focussed PRs.

Let's merge this right after another typo 'bit the dust' :).

src/mem.rs Outdated
///
/// All input data so far will be available to the decompressor (as with
/// `Flush::Sync`). This completes the current deflate block and follows it
/// with an empty fixed codes block that is 10 bites long, and it assures
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// with an empty fixed codes block that is 10 bites long, and it assures
/// with an empty fixed codes block that is 10 bytes long, and it assures

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am aware this typos wasn't introduced here, but we might as well fix it now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for your continued contributions, they are much appreciated!

Thank you for the kind words, I'm just doing what I can in my capabilities (:

@CosminPerRam CosminPerRam requested a review from Byron February 18, 2025 09:09
Copy link
Member

@Byron Byron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot, appreciated!

You seem to be studying the entire codebase and I am curious to see what else you will find :).

@Byron Byron merged commit 5a2fd04 into rust-lang:main Feb 19, 2025
14 checks passed
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.

2 participants