Re: Patches to java.util.zip by Christian Schlichtherle

[Mark Wielaard]

Hi,

On Sun, 2005-07-24 at 12:29 +0700, John Leuner wrote:
> Unfortunately I haven't been able to give this task the attention it
> deserves and I want to give someone else the opportunity to deal with
> these patches.
> 
> Below are some of the emails exchanged between Christian, Mark Wielaard
> and myself.

I went through what I reviewed earlier and committed all uncontroversial
stuff.

> > I didn't actually review the big part of the patch. And I don't think we
> > should add new public/protected constructors or methods. We should probably
> > move the new functionality to a new gnu.java.util.zip package with extended
> > functionality if possible.
> > 
> > But I quickly went through some of the things that I could quickly format
> > correctly and that I think can and should be submitted separately since they
> > are good changes on their own anyway.
> > 
> > Reformatted Adler32.java to follow out style guide and make the diff minimal
> > and updated copyright year. This can probably be submitted separately with a
> > little ChangeLog message plus a short explanation of the why of the changed
> > (any benchmarks?)

I haven't done any benchmarks myself. We probably should also have a
little mauve tests for Adler32 to make sure we don't introduce any bugs.
Cleaned up patch attached.

> > Dropped the change to java/util/zip/CRC32.java which I didn't understand
> > (what is CRC32If?):
> > -public class CRC32 implements Checksum
> > +public class CRC32 implements CRC32If
> > 
> > Same for the changes to java/util/zip/Deflater.java (what is
> > DeflaterIf?)
> > 
> > Dropped the change to java/util/zip/DeflaterEngine.java private methods are
> > always final:
> > -  private void updateHash() {
> > +  private final void updateHash() {
> > 
> > Added copyright year to java/util/zip/DeflaterHuffman.java and changed order
> > of modifiers to amtch coding styleguide. (Can also be submitted separately
> > as obvious fix with ChangeLog entry.)

Commited as:

2005-08-30  Christian Schlichtherle <address@hidden>

        * java/util/zip/DeflaterHuffman.java (bit4Reverse): Mark final.

> > Removed the changes to java/util/zip/DeflaterOutputStream.java that involved
> > DeflaterIf (which wasn't included and we cannot change the type and modifier
> > of a protected field). Are you sure you want to comment out/remove the
> > flush() method? (Can also be submitted separately with its own ChangeLog
> > entry.)

Dropped the removal of flush() but kept the increased buffer size.
Committed as:

2005-08-30  Christian Schlichtherle <address@hidden>

   * java/util/zip/DeflaterOutputStream.java
   (DeflaterOutputStream(OutputStream)): Increase buffer size to 4096.
   (DeflaterOutputStream(OutputStream,Deflater)): Likewise.

> > Updated the copyright year in java/util/zip/Inflater.java and removed the
> > implements InflaterIf. Updated documentation is obviously correct and can be
> > submitted separately with a ChangeLog entry.

A similar fix was already applied earlier:

2005-07-13  David Gilbert  <address@hidden>

        * java/util/zip/Inflater.java: minor API doc fixes.

> > Remove the changes to java/util/zip/InflaterInputStream.java which all had
> > to do with InflaterIf (not included).
> > 
> > The change to java/util/zip/OutputWindow.java is unnecessary since private
> > methods are already final:
> > -  private void slowRepeat(int rep_start, int len, int dist)
> > +  private final void slowRepeat(int rep_start, int len, int dist)
> > 
> > java/util/zip/ZipException.java had as only change an update to the
> > copyright years list. That is unnecessary.

I need to go through the rest of these patches.
It would help if they could be rediffed against current CVS head and
extra functionality would be moved to a new package.

Thanks,

Mark

Adler32.patch
Description: Text Data

DeflaterHuffman.patch
Description: Text Data

DeflaterOutputStream.patch
Description: Text Data

signature.asc
Description: This is a digitally signed message part