[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: 8034856/8034857: More gcc warnings
From: Mikael Vidstedt <mikael.vidstedt () oracle ! com>
Date: 2014-02-24 19:22:33
Message-ID: 530B9BF9.40102 () oracle ! com
[Download RAW message or body]
On 2014-02-23 01:19, Alan Bateman wrote:
> On 19/02/2014 18:22, Mikael Vidstedt wrote:
>> :
>>
>> The documented grammar in the comment only mentions "SPACE" and the
>> code below doesn't make any references to \t. As a matter of fact, it
>> only checks for one single, mandatory SPACE after the colon (enforced
>> at line 535-536) and doesn't care to remove any space characters at
>> the end of the value. The while loop only deals with continuations.
>> If additional spaces do exist they will as far as I can tell be part
>> of the value. Are they trimmed later? I'm assuming it would be nice
>> to have both parsers (parse_manifest & JarFacade) behave the same way?
>>
>> Here's what it would look like to only check for space, but still eat
>> any additional spaces which doesn't match what
>> parse_manifest/parse_nv_pair does:
>>
>> http://cr.openjdk.java.net/~mikael/webrevs/isspace/webrev.01/webrev/
>>
> Sorry for the delay getting back to you on this (I've been busy with
> other things).
>
> I checked the JAR File Specification, which is turn references RFC 822
> as the "inspiration" for the name-value pairs. The SPACE token is just
> ASCII SP. So I agree it's just ASCII SP that needs to be handled here,
> not LWSP-char which includes ASCII HT.
>
> Looking at JDK-6274276 then the trimming was done to avoid
> hard-to-diagnose problems with leading/trailing spaces. It's possible
> that this is inconsistent with other areas where JAR file attributes
> are used. I would suggest leaving it as is for now as this is
> potentially changing behavior in several areas.
That sounds reasonable. I'll keep the webrev.01 approach - only check
for and trim ASCII SP.
Thanks for the review!
Cheers,
Mikael
[Attachment #3 (text/html)]
<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<br>
<div class="moz-cite-prefix">On 2014-02-23 01:19, Alan Bateman
wrote:<br>
</div>
<blockquote cite="mid:5309BD25.2030407@oracle.com" type="cite">
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
On 19/02/2014 18:22, Mikael Vidstedt wrote:
<blockquote cite="mid:5304F64E.4000206@oracle.com" type="cite">
<meta content="text/html; charset=UTF-8"
http-equiv="Content-Type">
:<br>
<br>
The documented grammar in the comment only mentions "SPACE" and
the code below doesn't make any references to \t. As a matter of
fact, it only checks for one single, mandatory SPACE after the
colon (enforced at line 535-536) and doesn't care to remove any
space characters at the end of the value. The while loop only
deals with continuations. If additional spaces do exist they
will as far as I can tell be part of the value. Are they trimmed
later? I'm assuming it would be nice to have both parsers
(parse_manifest & JarFacade) behave the same way?<br>
<br>
Here's what it would look like to only check for space, but
still eat any additional spaces which doesn't match what
parse_manifest/parse_nv_pair does:<br>
<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Emikael/webrevs/isspace/webrev.01/webrev/">http://cr.openjdk.java.net/~mikael/webrevs/isspace/webrev.01/webrev/</a><br>
<br>
</blockquote>
Sorry for the delay getting back to you on this (I've been busy
with other things).<br>
<br>
I checked the JAR File Specification, which is turn references RFC
822 as the "inspiration" for the name-value pairs. The SPACE token
is just ASCII SP. So I agree it's just ASCII SP that needs to be
handled here, not LWSP-char which includes ASCII HT.<br>
<br>
Looking at JDK-6274276 then the trimming was done to avoid
hard-to-diagnose problems with leading/trailing spaces. It's
possible that this is inconsistent with other areas where JAR file
attributes are used. I would suggest leaving it as is for now as
this is potentially changing behavior in several areas.<br>
</blockquote>
<br>
That sounds reasonable. I'll keep the webrev.01 approach - only
check for and trim ASCII SP.<br>
<br>
Thanks for the review!<br>
<br>
Cheers,<br>
Mikael<br>
<br>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic