Thanks Jim! webrev has been updated as suggested. http://cr.openjdk.java.net/~sherman/8200530/webrev
-sherman On 06/05/2018 05:17 AM, Jim Laskey wrote:
Attributes.java:380 nit - assign c at decl - only test len if decremented byte c; if ((c = lbuf[--len]) != '\n'&& c != '\r') { throw new IOException("line too long"); } if (len> 0&& lbuf[len-1] == '\r') { --len; } if (len == 0) { break; } ====== byte c = lbuf[--len]; if (c != '\n'&& c != '\r') { throw new IOException("line too long"); } if (len> 0&& lbuf[len-1] == '\r'&& --len == 0) { break; } <<<<<< Manifest.java:208 nit - same as above byte c; if ((c = lbuf[--len]) != '\n'&& c != '\r') { throw new IOException("manifest line too long"); } if (len> 0&& lbuf[len-1] == '\r') { --len; } if (len == 0&& skipEmptyLines) { continue; } ====== byte c = lbuf[--len]; if (c != '\n'&& c != '\r') { throw new IOException("manifest line too long"); } if (len> 0&& lbuf[len-1] == '\r'&& --len == 0&& skipEmptyLines) { continue; } <<<<<< Manifest.java:396 nit - Shouldn’t c already be loaded with tbuf[tpos-1] (or tbuf[tpos-2]) if “\r\n”) if ((c = tbuf[tpos-1]) == '\n' || c == '\r') { break; } ====== if (c == '\n' || c == '\r') { break; } <<<<<< +1 Cheers, — JimOn Jun 5, 2018, at 2:38 AM, Xueming Shen<[email protected]> wrote: Hi, Please help review the changeset for JDK-8200530. "newline" is specified as |CR LF | LF | CR|(/not followed by/|LF|) in Jar spec [1] from the very beginning but our implementation actually never supports "\r"/CR (not followed by LF) case. The proposed change here is to add CR as an individual supported "newline"/line separator. issue: https://bugs.openjdk.java.net/browse/JDK-8200530 webrev: http://cr.openjdk.java.net/~sherman/8200530/webrev Thanks, Sherman [1] https://docs.oracle.com/javase/10/docs/specs/jar/jar.html
