Warnings Cleanup in java.util.<various> (more from hack day)

[Michael Barker]

Hi,

After some further review from the list and Martijn attached are
updated patches for Manifest.java and ZipEntry.java.  The unnecessary
import and the UTF8 changes are removed.  The ZipEntry now uses a
SuppressWarnings with a comment regarding the use of j.u.Calendar as
an alternative.

Apologies for the numerous round trips on this code.  Thank you to all
that reviewed and commented on the code and general style/process.
This is a bit of a learning experience for us (LJC).  It will make
further hack days much easier as we can better guide development and
review patches before sending them in.

Mike.

On Sat, Dec 3, 2011 at 11:05 AM, Martijn Verburg
<martijnverburg at gmail.com> wrote:
> Hi Sherman,
>
> In order to keep this change within the scope of the intentions of the
> exercise I'm going to revert that section to what it was (I'll re-spin a
> patch).  At this stage I won't add a @SuppressWarnings as I think this
> should be avoidable once it's looked at again in a little more depth.
>
> The rest follows in-line.  If this is starting to chew up your time then
> please don't feel obliged to answer, this is more for my own curiosities
> sake.  This also isn't a suggestion to change the patch again, just
> theorising here :-)
>
> On Friday, 2 December 2011, Xueming Shen <xueming.shen at oracle.com> wrote:
>> Martijn,
>>
>> The proposed change is incorrect.
>>
>> +  value = new String(vb, 0, 0, StandardCharsets.UTF_8);
>>
>> First, shouldn't it at least be
>>
>> value = new String(vb, StandardCharsets.UTF_8);
>>
>>  or
>>
>> value = new String(vb, 0, vb.length, StandardCharsets.UTF_8);
>>
>> Second, the "value" will be written out via dos.writeBytes(String) later,
> which
>> only takes the low-byte of a each char of the target String object.
>
> Right, so the need for the hi-byte constructor can be avoided, but by me
> ignoring the vb.length I introduced a bug, gotcha.
>
> So the code was:
>
> String value = e.getKey();
> if (value != null) {
>    byte[] vb = value.getBytes("utf8");
>    value = new String(vb, 0, 0, vb.length);
> }
>
> My proposed (bad) change was:
>
> String value = e.getKey();
> if (value != null) {
>    byte[] vb = value.getBytes(StandardCharsets.UTF_8);
>    value = new String(vb, 0, 0, StandardCharsets.UTF_8);
> }
>
> As you say, a better change would have been:
>
> String value = e.getKey();
> if (value != null) {
>    byte[] vb = value.getBytes(StandardCharsets.UTF_8);
>    value = new String(vb, 0, vb.length, StandardCharsets.UTF_8);
> }
>
> Or in a more performing manner:
>
> String value = e.getKey();
> if (value != null) {
>    // As at jdk8 build X - we don't use StandardCharsets.UTF_8 in getBytes
> for performance reasons
>    byte[] vb = value.getBytes("utf8");
>    value = new String(vb, 0, vb.length, StandardCharsets.UTF_8);
> }
>
>> That "not properly" conversion is actually what we need here to make sure
> the
>> output will be correctly in utf8. The only "reliable" replacement might
> be to use
>> "iso8859-1" (not utf_8), but I would recommend keep it un-touched.
>
> Fair enough.
>
> Thanks for clearing that up and apologies for causing pain :(
>
> Cheers,
> Martijn
-------------- next part --------------
diff -r 43a630f11af6 src/share/classes/java/util/jar/Manifest.java
--- a/src/share/classes/java/util/jar/Manifest.java	Wed Nov 30 13:11:16 2011 -0800
+++ b/src/share/classes/java/util/jar/Manifest.java	Sat Dec 03 12:53:27 2011 +0000
@@ -51,7 +51,7 @@
     private Attributes attr = new Attributes();
 
     // manifest entries
-    private Map entries = new HashMap();
+    private Map<String, Attributes> entries = new HashMap<>();
 
     /**
      * Constructs a new, empty Manifest.
@@ -148,11 +148,11 @@
         // Write out the main attributes for the manifest
         attr.writeMain(dos);
         // Now write out the pre-entry attributes
-        Iterator it = entries.entrySet().iterator();
+        Iterator<Map.Entry<String, Attributes>> it = entries.entrySet().iterator();
         while (it.hasNext()) {
-            Map.Entry e = (Map.Entry)it.next();
+            Map.Entry<String, Attributes> e = it.next();
             StringBuffer buffer = new StringBuffer("Name: ");
-            String value = (String)e.getKey();
+            String value = e.getKey();
             if (value != null) {
                 byte[] vb = value.getBytes("UTF8");
                 value = new String(vb, 0, 0, vb.length);
@@ -161,7 +161,7 @@
             buffer.append("\r\n");
             make72Safe(buffer);
             dos.writeBytes(buffer.toString());
-            ((Attributes)e.getValue()).write(dos);
+            e.getValue().write(dos);
         }
         dos.flush();
     }
-------------- next part --------------
diff -r 43a630f11af6 src/share/classes/java/util/zip/ZipEntry.java
--- a/src/share/classes/java/util/zip/ZipEntry.java	Wed Nov 30 13:11:16 2011 -0800
+++ b/src/share/classes/java/util/zip/ZipEntry.java	Sat Dec 03 12:55:05 2011 +0000
@@ -281,6 +281,8 @@
      * Converts DOS time to Java time (number of milliseconds since epoch).
      */
     private static long dosToJavaTime(long dtime) {
+        @SuppressWarnings("deprecation") // Changing to j.u.Calendar would have an
+                                         // unknown impact on performance/gc.
         Date d = new Date((int)(((dtime >> 25) & 0x7f) + 80),
                           (int)(((dtime >> 21) & 0x0f) - 1),
                           (int)((dtime >> 16) & 0x1f),
@@ -293,6 +295,8 @@
     /*
      * Converts Java time to DOS time.
      */
+    @SuppressWarnings("deprecation") // Changing to j.u.Calendar would have an
+                                     // unknown impact on performance/gc.
     private static long javaToDosTime(long time) {
         Date d = new Date(time);
         int year = d.getYear() + 1900;