Hi Sherman and everyone,
Thanks again that you already had a look at my last proposal and for
your most valuable input. While my originally intended changes for
6443578 and 6202130 are now [1] and [2] (still pending), I just wondered
whether the code you pointed out would be worth cleaning up with that
respect, presumably as a separate bug, task or patch. I'd also suggest
to reduce amount of duplicated code along with it.
That would at least stop people stumbling over the same pieces of code
again and again. Potentially, some of [3] could be re-used and with [1]
and [2], if accepted, there would be some test coverage already which
would be sufficient in my opinion.
Is that interesting implementation, duplicated portions of code, and use
of deprecated api worth another bug (or other reaction)? I haven't found
a specifically related bug and cannot create one myself and would be
glad if someone would create one if at all desired.
Regards,
Philipp
[1]
http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-May/052962.html
[2]
http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-May/052963.html
[3]
http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-May/052921.html
On 03.05.2018 08:56, Xueming Shen wrote:
Philipp,
I kinda recalled JDK-6202130 which I read long time ago and I believed
it's not a bug but
just wanted to leave it open and for double check, then it's off the
priority list somehow...
Reread the code below I think it handles utf8 and the 72-length-limit
appropriately, though
a little tricky,
(1) value.getByte("utf8") + deprecated String(byte[], int, int, int);
which creates a String that each "char" inside that String object
actually represents one byte
value of the resulted utf8 byte sequence, with its utf16 value =
[hi-byte= 0 and low-byte="utf8-byte"]
(2) append this String to the StringBuffer (--> StringBuilder), now
the sb.length() actually is the
length of the utf8 byte sequence, make72Safe(...) is used to
adjust the length to 72 working
on "chars" inside the buffer.
(3) write out the adjusted buffer via DataOutputStream.writeBytes()
in which the "write" cuts off the hi-byte of that utf16, so you
actually get the original
utf-8 byte sequence output to the stream.
Sure the implementation looks "interesting" and uses deprecated String
constructor, but it was
written 2 decades ago.
Back then I think we were thinking maybe the fix is to simply remove
the "misleading" comment
line below in the source code, if the implementation actually supports
utf-8 + 72-limit.
" * XXX Need to handle UTF8 values and break up lines longer than
72 bytes"
Any reason/test case to believe the mechanism does not work?
BTW the logic of line below in writeMain() appears to be incorrect. I
do have another bug for it.
" if ((version != null) &&
!(name.equalsIgnoreCase(vername))) {"
----------------------
StringBuffer buffer = new StringBuffer(name);
buffer.append(": ");
String value = (String) e.getValue();
if (value != null) {
byte[] vb = value.getBytes("UTF8");
value = new String(vb, 0, 0, vb.length);
}
buffer.append(value);
Manifest.make72Safe(buffer);
buffer.append("\r\n");
out.writeBytes(buffer.toString());
-------------------------
sherman
On 5/1/18, 10:21 PM, Philipp Kunz wrote:
Hi,
Recently, I tried to fix only bug 6202130 with the intention to fix
bug 6443578 later with the intention to get some opportunity for
feedback, but haven't got any, and propose now a fix for both
together which in my opinion makes more sense.
See attached patch.
Some considerations, assumptions, and explanations
* In my opinion, the code for writing manifests was distributed in the
two classes Attributes and Manifest in an elegant way but somewhat
difficult to explain the coherence. I chose to group the code that
writes manifests into a new class ManifestWriter. The main incentive
for that was to prevent or reduce duplicated code I would have had
to change twice otherwise. This also results in a source file of a
suitable size.
* I could not support the assumption that the write and writeMain
methods in Attributes couldn't be referenced anywhere so I
deprecated them rather than having them removed.
* I assumed the patch will not make it into JDK 10 and, hence, the
deprecated annotations are attributed with since = 11.
* I could not figure out any reason for the use of DataOutputStream
and did not use it.
* Performance-wise I assume that the code is approximately comparable
to the previous version. The biggest improvement in this respect I
hope comes from removing the String that contains the byte array
constructed with deprecated String(byte[], int, int, int) and then
copying it over again to a StringBuffer and from there to a String
again and then Characters. On the other hand, keeping whole
characters together when breaking lines might make it slightly
slower. I hope my changes are an overall improvement, but I haven't
measured it.
* For telling first from continuation bytes of utf-8 characters apart
I re-used a method isNotUtfContinuationByte from either StringCoding
or UTF_8.Decoder. Unfortunately I found no way not to duplicate it.
* Where it said before "XXX Need to handle UTF8 values and break up
lines longer than 72 bytes" in Attributes#writeMain I did not dare
to remove the comment completely because it still does not deal
correctly with version headers longer than 72 bytes and the set of
allowed values. I changed it accordingly. Two similar comments are
removed in the patch.
* I added two tests, WriteDeprecated and NullKeysAndValues, to
demonstrate compatibility as good as I could. Might however not be
desired to keep and having to maintain.
* LineBrokenMultiByteCharacter for jarsigner should not be removed or
not so immediately because someone might attempt to sign an older
jarfile created without that patch with a newer jarsigner that
already contains it.
suggested changes or additions to the bug database: (i have no
permissions to edit it myself)
* Re-combine copies of isNotUtfContinuationByte (three by now).
Relates to 6184334. Worth to file another issue?
* Manifest versions have specific specifications, cannot break across
lines and can contain a subset of characters only. Bug 6910466
relates but is not exactly the same. If someone else is convinced
that writing a manifest should issue a warning or any other way to
deal with a version that does not conform to the specification, I'd
suggest to create a separate bug for that.
Now, I would be glad if someone sponsored a review. This is only my
third attempt to submit a patch which is why I chose a lesser
important subject to fix in order to get familiar and now I
understand it's not the most attractive patch to review. Please don't
hesitate to suggest what I could do better or differently.
As a bonus, with these changes, manifest files will always be
displayed correctly with just any utf capable viewer even if they
contain multi-byte utf characters that would have been broken across
a line break with the current/previous implementation and all
manifests will become also valid strings in Java.
