Repository navigation
Conversation
8a973f7 to
312e60d
Compare
ygree
left a comment
There was a problem hiding this comment.
I noticed a couple of issues with the current glue bytecode packing scheme: it treats arbitrary byte pairs as UTF-16BE code units, which means the generated bytecode has to be padded to an even length and decoding can run into invalid/surrogate UTF-16 values unless handled carefully. That makes the implementation more subtle than it needs to be.
I spiked an alternative Latin-1 implementation in #124 where each byte maps directly to one character and decoding is just String.getBytes(ISO_8859_1). This removes the padding requirement and avoids the pseudo-UTF-16BE/custom-decoding concern.
I approve, and I'll leave it up to you to decide which approach to use.
|
@ygree yes, your approach does make more sense for the glue we're generating, so I'm going to drop my PR. Originally I was concerned that we could end up with very large string allocations, but it turned out we only need to use this approach for a limited number of classes. And even the extra ones I'll be adding soon are not large enough that using ISO_8859_1 would be that noticeable, as you point out. One ask - currently I can't merge your PR because the commit is missing a verified signature - can you fix that? |
What Does This Do
Our current
defineClassglue is safe to unpack usinggetBytes(UTF_16BE)because it has no unpaired surrogates.However we should make sure the general
Glue.unpackBytecodehelper can tolerate non-trivial glue that may contain unpaired surrogates. To do this we unpack the characters into bytes ourselves, avoiding the JDK's built-in surrogate validation.Contributor Checklist
Jira ticket: [PROJ-IDENT]