Harmony
  1. Harmony
  2. HARMONY-158

Two JSE 5 methods are not implemented in java.util.zip.Deflator

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None
    • Estimated Complexity:
      Moderate

      Description

      Two methods:
      java/util/zip/Deflater public getBytesRead()J
      java/util/zip/Deflater public getBytesWritten()J
      are introduced by JSE 5, but they are not been implemented by Harmony yet.

      1. 158-v2.zip
        3 kB
        Paulex Yang
      2. 158-native-patch.txt
        7 kB
        Paulex Yang
      3. java.util.zip.Deflater.patch
        2 kB
        Paulex Yang
      4. java.util.zip.DeflaterTests.patch
        2 kB
        Paulex Yang

        Issue Links

          Activity

          Hide
          Paulex Yang added a comment -

          The patches are attached, one for java implementation, one for test, and the last for native codes. Pls. note that the native patches include both win.IA32 and linux.IA32, so pls. apply them on level of "native-src" directory.

          Show
          Paulex Yang added a comment - The patches are attached, one for java implementation, one for test, and the last for native codes. Pls. note that the native patches include both win.IA32 and linux.IA32, so pls. apply them on level of "native-src" directory.
          Hide
          George Harley added a comment -

          Thanks Paulex,

          Applied patches to archive module and to native-src at repo revision 391073.

          I have some observations to make on this :

          1) The native source patch code for this issue also made updates to the
          inflater code which forced me to update inflater.c as well as deflater.c or
          otherwise break the build. On closer inspection, it looks like this issue's
          patch actually contains code that was intended for HARMONY-159 (see the updates
          to the jclprots.h source). As HARMONY-158 and HARMONY-159 are so closely
          related I am committing the fixes for both at the same time. It would have been
          smoother had the patches actually been separate - assuming I have understood
          the problem correctly here.

          2) The patch to java/util/zip/Inflater.java does not update the signature of
          the native method getTotalOutImpl to reflect the change of return type from int
          to long. That looks like an ommission. So, instead of ...

          private native synchronized int getTotalOutImpl(long handle);

          ...it looks like it should be ...

          private native synchronized long getTotalOutImpl(long handle);

          It's minor but I have remedied this in the committed version of
          java/util/zip/Inflater.java .

          3) I have a gut feeling that there could be more test code for these changes.
          What do you think ?

          4) The Javadoc comments for the new methods (e.g. getBytesRead) don't contain much explanation. What do you think ?

          Please check that the patch was applied as you expected.

          Best regards,
          George

          Show
          George Harley added a comment - Thanks Paulex, Applied patches to archive module and to native-src at repo revision 391073. I have some observations to make on this : 1) The native source patch code for this issue also made updates to the inflater code which forced me to update inflater.c as well as deflater.c or otherwise break the build. On closer inspection, it looks like this issue's patch actually contains code that was intended for HARMONY-159 (see the updates to the jclprots.h source). As HARMONY-158 and HARMONY-159 are so closely related I am committing the fixes for both at the same time. It would have been smoother had the patches actually been separate - assuming I have understood the problem correctly here. 2) The patch to java/util/zip/Inflater.java does not update the signature of the native method getTotalOutImpl to reflect the change of return type from int to long. That looks like an ommission. So, instead of ... private native synchronized int getTotalOutImpl(long handle); ...it looks like it should be ... private native synchronized long getTotalOutImpl(long handle); It's minor but I have remedied this in the committed version of java/util/zip/Inflater.java . 3) I have a gut feeling that there could be more test code for these changes. What do you think ? 4) The Javadoc comments for the new methods (e.g. getBytesRead) don't contain much explanation. What do you think ? Please check that the patch was applied as you expected. Best regards, George
          Hide
          Paulex Yang added a comment -

          The codes look fine, thank you, George.

          And thank you special for your detailed observation and comments, my comments below:

          >> 1) The native source patch code for this issue also made updates to the
          .....
          Yes, the two patches are relevant, sorry for that, seems the two JIRA should be merged as one, because they need to modify same native files. I shall try to be more considerate later.

          >>2) The patch to java/util/zip/Inflater.java does not update the signature of
          ......
          Yes, you are right again. It's my fault on this.

          >>3) I have a gut feeling that there could be more test code for these changes.
          ......
          I didn't include them because in fact the new methods are only extended version of some original methods, like getTotalIn(), etc, so the behavior should be same except when the total bytes are larger than Integer.MAX_VALUE, but IMHO this case is hard to test(we may need a huge zip file...). But you are right that it still needs more test.

          4) The Javadoc comments for the new methods (e.g. getBytesRead) don't contain much explanation. What do you think ?
          ......
          Because of the same reason above, the Java doc is short, but I guess I should more explicitly declare the relationship between these new methods and the old ones(currently it is just implied by @see)

          I'm willing to create another patch to improve the points above. Your comments?

          Show
          Paulex Yang added a comment - The codes look fine, thank you, George. And thank you special for your detailed observation and comments, my comments below: >> 1) The native source patch code for this issue also made updates to the ..... Yes, the two patches are relevant, sorry for that, seems the two JIRA should be merged as one, because they need to modify same native files. I shall try to be more considerate later. >>2) The patch to java/util/zip/Inflater.java does not update the signature of ...... Yes, you are right again. It's my fault on this. >>3) I have a gut feeling that there could be more test code for these changes. ...... I didn't include them because in fact the new methods are only extended version of some original methods, like getTotalIn(), etc, so the behavior should be same except when the total bytes are larger than Integer.MAX_VALUE, but IMHO this case is hard to test(we may need a huge zip file...). But you are right that it still needs more test. 4) The Javadoc comments for the new methods (e.g. getBytesRead) don't contain much explanation. What do you think ? ...... Because of the same reason above, the Java doc is short, but I guess I should more explicitly declare the relationship between these new methods and the old ones(currently it is just implied by @see) I'm willing to create another patch to improve the points above. Your comments?
          Hide
          George Harley added a comment -

          Hi Paulex,

          An additional patch to address these points would be welcome. I will reopen the issue now.

          Best regards,
          George

          Show
          George Harley added a comment - Hi Paulex, An additional patch to address these points would be welcome. I will reopen the issue now. Best regards, George
          Hide
          George Harley added a comment -

          Awaiting further patches.

          Show
          George Harley added a comment - Awaiting further patches.
          Hide
          Paulex Yang added a comment -

          George, here goes the the version 2 patch with more tests and documents, as well as some minor changes (copyright year, etc). Pls. try it following these steps:
          1. apply 01.158.src.diff on src/main
          2. run 02.158.sh to add new test file
          3. apply 03.158.test.diff on src/test

          Show
          Paulex Yang added a comment - George, here goes the the version 2 patch with more tests and documents, as well as some minor changes (copyright year, etc). Pls. try it following these steps: 1. apply 01.158.src.diff on src/main 2. run 02.158.sh to add new test file 3. apply 03.158.test.diff on src/test
          Hide
          George Harley added a comment -

          Hi Paulex,

          New changes committed in revision 392984.

          Please could you check that the patch has been applied as expected.

          Thanks,
          George

          Show
          George Harley added a comment - Hi Paulex, New changes committed in revision 392984. Please could you check that the patch has been applied as expected. Thanks, George
          Hide
          Paulex Yang added a comment -

          looks good, thank you, George.

          Show
          Paulex Yang added a comment - looks good, thank you, George.
          Hide
          George Harley added a comment -

          Verified by Paulex.

          Show
          George Harley added a comment - Verified by Paulex.

            People

            • Assignee:
              George Harley
              Reporter:
              Paulex Yang
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development