+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
