Hi Clemens,
Thanks for the code drop! I was not yet able to spend much Quality Time with
it. At a glance, it seems like the fix might work. The general ideas seem
sound, and I appreciate your effort in addressing it. So please don't take
the feedback below as anything but encouragement to help get this bug fixed!
That said...it is a low priority bug (for us, anyway; a P4 out of 5) and we
have bigger fish to fry. I'll attend to it as time permits... Then too, there
are some issues to resolve re the provided code. I know it's an "Early
version", but some changes to it would make it easier to examine.
For example, Deflater.java was completely reformatted. When diffing the code,
every changeof indentation, javadoc removal, spaces to tabs, moving of {, etc.
shows up. We don't allow such changes into the JDK sources. Spaces only, and
despite whatever awful "standards" (or not!) already in use in the file,
please stick to them. Make every change be the best/smallest one which
directly addresses the bug. This makes it easier for all concerned to examine
the relevant changes. There are similar issues in Deflater.c.
The files provided the lack the GNU copyright file headers. My guess is that
they originated in the src.zip of a binary distribution. Regardless of their
source, could you please instead use files from mercurial repository at
hg.openjdk.java.net/jdk7/tl?
The above will make it easier to review future changes to the files. Enough
of that boring stuff, here's some feedback on "interesting" part of the changes!
In Deflater.java, new method rangeCheck() is used in a couple of places, but
it is not an adequate replacement for the code previously in
Deflater.setDictionary(); it incompatibly changes the error condition checking
semantics (we can't omit the check on strm). We strive to not introduce
incompatible changes, even small ones like this.
Another incompatible change is to the semantics of Deflater.deflate()...I
think. In Deflater.c, it seems that deflateBytes will use setParams if
necessary and then compress...I reference your email about 6539727 in this
regard (You are completely right about that being a non-bug, BTW and I'll
update it shortly: thanks!) I think your solution would have been the Right
Thing to do way back when, but we don't want to make an incompatible change
now. (I haven't reviewed this thoroughly enough; see my notes re formatting &
priorities above.)
With a change of this sort, we really do need tests along with a fix. Have
you started writing any test cases?
Finally, it seems that this solution obviates the need for the striding in
DeflaterOutputStream...IIRC, that is some of the original motivation for this
work. If you have suggested changes to that class as well, please include them.
I appreciate the work you've put in, and again, I hope to not dissuade you.
But we have certain standards to which we must adhere, and it's a not a very
high priority for us now, so we have to minimize the time we spend on it.
Thanks,
Dave
Clemens Eisserer wrote:
Hello again,
I've finished an early version of the java.util.zip.Deflater
implementation that uses striding. Its in an early stage and quite
likely will be buggy.
It passes FlaterTest and a simple test written by myself, but maybe
acts differently in corner-cases.
I would be happy to receive some comments as well as criticism ;)
lg Clemens
PS: Sorry for the traffic lately.
