Skip to content

Support round-tripping non trivial glue - #121

Closed
mcculls wants to merge 2 commits into
mainfrom
mcculls/fix-non-trivial-glue-roundtrip
Closed

mcculls wants to merge 2 commits into
mainfrom
mcculls/fix-non-trivial-glue-roundtrip

Conversation

@mcculls

@mcculls mcculls commented Sep 25, 2026

Copy link
Copy Markdown
Collaborator

What Does This Do

Our current defineClass glue is safe to unpack using getBytes(UTF_16BE) because it has no unpaired surrogates.

However we should make sure the general Glue.unpackBytecode helper 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]

@mcculls
mcculls marked this pull request as ready for review September 25, 2026 15:39
@mcculls
mcculls requested a review from a team as a code owner September 25, 2026 15:39
@mcculls
mcculls requested review from ygree and removed request for a team September 25, 2026 15:39
@mcculls
mcculls force-pushed the mcculls/fix-non-trivial-glue-roundtrip branch from 8a973f7 to 312e60d Compare September 25, 2026 19:50

@ygree ygree left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@mcculls

mcculls commented Sep 26, 2026 •

Copy link
Copy Markdown
Collaborator Author

@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?

@mcculls mcculls closed this Sep 26, 2026
@mcculls
mcculls deleted the mcculls/fix-non-trivial-glue-roundtrip branch September 26, 2026 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants