-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
8326099: GZIPOutputStream should use Deflater.getBytesRead() instead of Deflater.getTotalIn() #17900
Conversation
…nstead of Deflater.getTotalIn()
👋 Welcome back eirbjo! A progress list of the required criteria for merging this PR into |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello Eirik, the change looks OK to me.
For context, the getTotalIn()
and getTotalOut()
methods are being proposed for deprecation https://mail.openjdk.org/pipermail/core-libs-dev/2024-February/119321.html. getTotalIn()
was doing the same (int) getBytesRead()
internally, so this change should be OK.
@eirbjo This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 186 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
Thanks for veryfying and adding helpful context! I'll let this linger just a bit before integrating in case there is any non-correctness feedback, like about the added comment to clarify the modulo 2^32 aspect. |
/integrate |
Going to push as commit bb6b048.
Your commit was automatically rebased without conflicts. |
Please review this cleanup PR in preparation for deprecating
Deflater.getTotalIn()
in JDK-8326096.This PR replaces
GZIPOutputStream.writeTrailer
's call toDeflater.getTotalIn()
with a call toDeflater.getBytesRead()
followed by an explicit conversion to "modulo 2^32" (a cast to int) as described in RFC 1952:Testing and verification: This should be trivially verifiable by code inspection. Nevertheless, I wrote a test which writes Integer.MAX_VALUE +1 bytes of uncompressed data and verified that the last four bytes written to the file was indeed as expected. (This test is not included in this PR because of its runtime and resource requirements).
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/17900/head:pull/17900
$ git checkout pull/17900
Update a local copy of the PR:
$ git checkout pull/17900
$ git pull https://git.openjdk.org/jdk.git pull/17900/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 17900
View PR using the GUI difftool:
$ git pr show -t 17900
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/17900.diff
Webrev
Link to Webrev Comment