I only took a quick look. Looks good, but here's a nitpick - capitalize javadoc that begins with "returns"
On Fri, Oct 19, 2018 at 1:27 AM, Philipp Kunz <philipp.k...@paratix.ch> wrote: > Hi Martin and everyone, > > You were absolutely right to object "utf". > Please find a revised and updated patch attached. > > Regards, > Philipp > > > > On Thu, 2018-10-04 at 15:24 -0700, Martin Buchholz wrote: > > "utf8" is a thing. "utf" is not. > > > > On Thu, Oct 4, 2018 at 12:58 PM, Philipp Kunz <philipp.kunz@paratix.c > > h> wrote: > > > 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 >
