Review Request: 6202130: java.util.jar.Attributes.writeMain() can't handle multi-byte chars

[Philipp Kunz]

Thanks to Sherman's hint, I revised the test to use the terms unicode
character and utf encoding consistently and not utf character. Affects
only the test, mostly in comments and a few identifiers.

Philipp


On Wed, 2018-10-03 at 12:48 -0700, Xueming Shen wrote:
> +1
> 
> only nit is it might be preferred to use "utf8" directly in the test
> instead of current "utf"
> 
> -sherman
> 
> On 10/03/2018 11:55 AM, Philipp Kunz wrote:
> > In short, bug 6202130 is about removal of comments in Attributes
> > like
> > 
> > > XXX Need to handle UTF8 values and break up lines longer than 72
> > 
> > The patch here contains a test that shows that utf characters are
> > handled correctly, which has been the case before. Breaking lines
> > has
> > also been working before.
> > 
> > In the main code, the patch only removes a few comments and does
> > not
> > change actual code. It all has been working before and now we know.
> > 
> > Philipp
> > 
> > 
> > https://bugs.openjdk.java.net/browse/JDK-6202130
> > 
> > 
> > 
> > 
> > On Wed, 2018-09-26 at 07:08 +0200, Philipp Kunz wrote:
> > > Hi,
> > > 
> > > There are some comments in the source code like this:
> > > 
> > >   >  XXX Need to handle UTF8 values and break up lines longer
> > > than 72 bytes
> > > 
> > > in Attributes.java on lines 299, 327, and 370. Bug 6202130 also
> > > suggests
> > > to add a test to show that the character set is fully supported
> > > before
> > > actually removing those comments.
> > > 
> > > Hence, I added such a test, see ValueUtfEncoding in attached
> > > patch, and
> > > it finds the complete utf character set supported correctly and
> > > that it
> > > has been so already before.
> > > 
> > > The comments in Attributes also say that lines longer than 72
> > > bytes need
> > > to be broken. That is a different issue and has been already
> > > addressed
> > > and solved in bug 6372077 and is now tested in LineBreakLineWidth
> > > and
> > > therefore, that line break part of the comment is obsolete now as
> > > well
> > > with the exception of version headers ("Manifest-Version" or
> > > "Signature-Version") at least when taking strictly.
> > > 
> > > Version headers are not subject of bug 6202130 for which my patch
> > > intends to provide a fix and if only because multi-byte
> > > characters are
> > > not allowed in version headers and wasn't subject of 6372077
> > > either
> > > which was about too short lines rather than too long ones. If I
> > > would
> > > change only the minimum I could argue was safe, there would have
> > > to
> > > remain a comment in Attributes.java on line 327 like
> > > 
> > >   >  XXX Need to handle version headers longer than 72 bytes
> > > 
> > > but I guess that should probably better be represented as another
> > > bug
> > > rather than this style of comment if at all desired. There are
> > > some bugs
> > > concerning version headers and particularly the behavior of the
> > > manifests in their absence, among possibly others, 6910466,
> > > 4935610,
> > > 4271239, 8196371 and 4256580 which have some relation but I
> > > haven't
> > > found one that really fits the 72 byte line length limit of
> > > version
> > > headers. Whether such a bug is created or found or not, it does
> > > not
> > > directly relate to the one I'm trying to fix here, 6202130.
> > > 
> > > If also the test is good I would assume that the comments can be
> > > removed now without actually changing the Manifest and Attributes
> > > implementation.
> > > 
> > > Regards,
> > > Philipp
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 6202130.patch
Type: text/x-patch
Size: 9318 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/core-libs-dev/attachments/20181004/9698f36f/6202130-0001.patch>