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

Never return the internal module name as a mutable string. #5481

Merged
merged 2 commits into from Nov 29, 2018

Conversation

@headius
Copy link
Member

@headius headius commented Nov 29, 2018

This duplicates logic in to_s that dups the module name before
returning it, but also ensures that any cached module name is
frozen so it can't be modified in place.

Fixes #5480.

@headius headius added this to the JRuby 9.2.5.0 milestone Nov 29, 2018
This duplicates logic in `to_s` that dups the module name before
returning it, but also ensures that any cached module name is
frozen so it can't be modified in place.

Fixes #5480.
@headius headius force-pushed the headius:no_mutable_class_names branch from e4e8d81 to d3b31dd Nov 29, 2018
@enebo enebo merged commit d0cd9a2 into jruby:master Nov 29, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@enebo
Copy link
Member

@enebo enebo commented Nov 29, 2018

@headius this sort of makes sense how it broke. Because we were a string before we would always make a RubyString from the string and we did not cache that finished string. Not sure if we always marked it frozen or not before.

@headius
Copy link
Member Author

@headius headius commented Dec 1, 2018

@enebo I didn't see any evidence that we froze it, but it makes sense to do so. That inner method was added by you in 9.2 so it seems safe enough to make it now return a frozen string reference. to_s already did a dup on it before returning, so name was just in error.

@headius headius deleted the headius:no_mutable_class_names branch Dec 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants