On Fri, 11 Oct 2024 14:05:40 GMT, Lance Andersen <lan...@openjdk.org> wrote:
> I think we need to be a bit cautious with the proposed change while I > understand the desire to remove potentially unneeded code. I understand the need to be cautious in this area. But I would also like to avoid cargo-culting this for another 21 years :-) > I have not yet found a specific issue for the for the original need for the > overload of fill, but have not looked to hard either If we trust the zlib change log, the original need retired with 1.2.0. Any of the performance-motivated forks like chromium-zlib or cloudflare-zlib should have been forked long after this. Perhaps @simonis can confirm. >, and the constructor in Inflater.java also makes reference to gzip for which >we do not have as much test coverage as we do for Zip. It's interesting that `GZIPInputStream` does _not_ override `InflaterInputStream.fill`, but _does_ use `nowrap`. The extra dummy byte note in the Inflater constructor is now misleading and should probably go too. Would require a CSR. The `@param nowrap` note is perhaps also misleading in that it is not actually specific to GZIP. It just tells zlib to expect raw deflated data, without any wrapping, GZIP or zlib. I guess since the only wrapping supported by the JDK is GZIP, the note is maybe okay. But if we do a CSR, I think we should relax this note to not reference GZIP. > So we might want to hold off on this PR for JDK 24 while we try to do some > more archaeological digging. That is fair. > As far as which zlib with the openJDK, currently the openjdk project bundles > zlib with the windows builds and relies on the zlib bundled with the various > OS's > > I would be surprised to find any vendor using a zlib below zlib 1.2.13 or > 1.3.1 Yes, that would indeed be awkward. ------------- PR Comment: https://git.openjdk.org/jdk/pull/21467#issuecomment-2407549193
