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.shen at 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