Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issue where AsciiStringRef.set() and UTF8StringRef.set() would throw an IllegalArgumentException. #276

Merged
merged 1 commit into from
Nov 22, 2021

Conversation

charleskorn
Copy link
Contributor

Fixes #159.

I've added a basic test case for the scenario that fails without this fix and passes with the fix.

I'm not sure if my changes are correct though:

  • should UTFStringRef.set() also update this.length to the length of the new string value?
  • should we be allocating additional space for the null terminator character in the call to getRuntime().getMemoryManager().allocateDirect()?

@headius
Copy link
Member

headius commented Oct 26, 2021

  • should UTFStringRef.set() also update this.length to the length of the new string value?
  • should we be allocating additional space for the null terminator character in the call to getRuntime().getMemoryManager().allocateDirect()?

These are good questions but perhaps they are not related to this fix? In other words, I think both of these would have been issues before, so we don't need to address them as part of your fix... maybe?

The fix seems right. We should investigate the above issues and try to get answers there, but I think we can merge this without addressing them right now.

What do you think?

@headius headius modified the milestones: 2.2.7, 2.2.8, 2.2.9 Oct 26, 2021
@charleskorn
Copy link
Contributor Author

Up to you, you know this stuff much better than I do @headius :)

Are you able to pick up the investigation of those two items? I don't know enough about the expected / desired behaviour to investigate them properly.

@headius
Copy link
Member

headius commented Oct 26, 2021

I will try to investigate the two issues soon and get a new release out... might take a bit to circle back to this though.

@charleskorn
Copy link
Contributor Author

Any chance this could be merged and released @headius?

@headius
Copy link
Member

headius commented Nov 22, 2021

Yeah, I've been under the weather and finally getting back to work. Could you open additional issues for those open questions you have? I will merge this now.

@charleskorn
Copy link
Contributor Author

No worries at all, I've opened an issue for them: #285

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.

UTFStringRef.set() throws IllegalArgumentException
2 participants