Warnings Cleanup in java.util. (more from hack day) (original) (raw)
Michael Barker mikeb01 at gmail.com
Sat Dec 3 05:03:32 PST 2011
- Previous message: Warnings Cleanup in java.util. (more from hack day)
- Next message: Warnings Cleanup in java.util. (more from hack day)
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
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.UTF8); First, shouldn't it at least be value = new String(vb, StandardCharsets.UTF8); or value = new String(vb, 0, vb.length, StandardCharsets.UTF8); 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.UTF8); value = new String(vb, 0, 0, StandardCharsets.UTF8); } As you say, a better change would have been: String value = e.getKey(); if (value != null) { byte[] vb = value.getBytes(StandardCharsets.UTF8); value = new String(vb, 0, vb.length, StandardCharsets.UTF8); } Or in a more performing manner: String value = e.getKey(); if (value != null) { // As at jdk8 build X - we don't use StandardCharsets.UTF8 in getBytes for performance reasons byte[] vb = value.getBytes("utf8"); value = new String(vb, 0, vb.length, StandardCharsets.UTF8); } 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 utf8), 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);
} -------------- 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) {e.getValue().write(dos); } dos.flush();@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
private static long javaToDosTime(long time) { Date d = new Date(time); int year = d.getYear() + 1900;// unknown impact on performance/gc.
- Previous message: Warnings Cleanup in java.util. (more from hack day)
- Next message: Warnings Cleanup in java.util. (more from hack day)
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]