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

Don't freeze input strings #28729

Merged
merged 1 commit into from
Apr 11, 2017
Merged

Conversation

matthewd
Copy link
Member

See 34321e4 for background on ImmutableString vs String.

Our String type cannot delegate typecasting to ImmutableString, because the latter freezes its input: duplicating the value after that gives us an unfrozen result, but still mutates the originally passed object.

Fixes #24185
Fixes #28718

See 34321e4 for background on
ImmutableString vs String.

Our String type cannot delegate typecasting to ImmutableString, because
the latter freezes its input: duplicating the value after that gives us
an unfrozen result, but still mutates the originally passed object.
::String.new(super)
case value
when ::String then ::String.new(value)
when true then "t".freeze
Copy link
Member

Choose a reason for hiding this comment

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

It just sucks that we have to duplicate the conditionals and the true and false cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

True, but it seems the rate of change on these branches is fairly low.

@matthewd matthewd merged commit fff414e into rails:master Apr 11, 2017
matthewd added a commit that referenced this pull request Apr 11, 2017
matthewd added a commit that referenced this pull request Apr 11, 2017
@matthewd
Copy link
Member Author

1949a25, 1fb87ea

@matthewd matthewd deleted the dont-freeze-inputs branch June 11, 2017 23:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants