Re: RFR JDK-8200530: '\r' is not supported as "newline" in java.util.jar.Manifest.

[Xueming Shen]

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,

— Jim

On Jun 5, 2018, at 2:38 AM, Xueming Shen<xueming.s...@oracle.com>  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