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

Handle symbol dumping properly #5528

Merged
merged 4 commits into from Dec 18, 2018

Conversation

headius
Copy link
Member

@headius headius commented Dec 18, 2018

This PR is for #5523. In that issue, we found that the order a symbol and its encoding are being registered are correct, but when unmarshaling the symbol we register the encoding first. This led to the actual symbol content being treated as the encoding key, and the encoding value became the hash value associated to the original symbol. Then everything was off by one.

There's an issue here, though, in that the symbol is created and registered with our internal tables before its encoding is set correctly. This is also how MRI works, but they don't have issues for obvious reasons.

@headius headius added this to the JRuby 9.2.6.0 milestone Dec 18, 2018
@headius
Copy link
Member Author

headius commented Dec 18, 2018

The one failure appears to be related to #5520 and not anything in this PR.

This also avoids modifying the encoding of an existing symbol,
which was possible in the previous logic.
@headius
Copy link
Member Author

headius commented Dec 18, 2018

Additional work brought about by @enebo realizing the previous logic could potentially change the encoding of an existing symbol. I added a new newSymbol path that accepts a "handler" lambda, called with the existing symbol + false (not a new symbol) or the new symbol + true (new symbol) before storage into the symbol table. This avoids both modifying an existing symbol's encoding and allows us to register the symbol link before setting the encoding.

@headius
Copy link
Member Author

headius commented Dec 18, 2018

I believe @enebo approves.

@headius headius merged commit 3bc7662 into jruby:master Dec 18, 2018
@headius headius deleted the fix_hash_dump_with_symbols branch December 18, 2018 18:12
@headius headius restored the fix_hash_dump_with_symbols branch December 18, 2018 18:12
@headius headius deleted the fix_hash_dump_with_symbols branch December 18, 2018 18:12
@headius headius restored the fix_hash_dump_with_symbols branch December 18, 2018 18:12
@headius headius deleted the fix_hash_dump_with_symbols branch December 18, 2018 18:12
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.

None yet

1 participant