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 rb_interned_str_* functions to not assume static strings #3786

Merged
merged 1 commit into from
Nov 30, 2020

Conversation

casperisfine
Copy link
Contributor

Followup: #3586

When passed a fake_str, register_fstring would create new strings with str_new_static. That's not what was expected, and answer almost no use cases.

@casperisfine casperisfine force-pushed the rb-interned-str-copy branch 2 times, most recently from 350cd2e to fd19844 Compare November 18, 2020 15:31
@XrXr
Copy link
Member

XrXr commented Nov 18, 2020

It makes sense to me that these functions should work on buffers that are not C string literals.

In that case, we are creating some new APIs instead of merely exposing some previously internal ones without modification, meaning the title of the Redmine ticket is kind of a misnomer. I bring this up to make sure that everyone is on the same page about what these new functions are supposed to do. I didn't follow the discussion for the ticket.

Code wise, I would prefer passing an additional parameter to fstr_update_callback instead of copy pasting and modifying it.

@casperisfine
Copy link
Contributor Author

Code wise, I would prefer passing an additional parameter to fstr_update_callback instead of copy pasting and modifying it.

Yes that a good point. I'll try to clean this up.

Fixes [Feature #13381]

When passed a `fake_str`, `register_fstring` would create new strings
with `str_new_static`. That's not what was expected, and answer
almost no use cases.
@casperisfine
Copy link
Contributor Author

@ko1 I'm sorry to bother you again with this but could you have a look? Without this PR I don't think rb_interned_str* is usable.

@ko1
Copy link
Contributor

ko1 commented Nov 19, 2020

Thank you I'm asking @nobu

@casperisfine
Copy link
Contributor Author

Thanks @nobu !

@casperisfine
Copy link
Contributor Author

@ko1 is this good for you?

@ko1
Copy link
Contributor

ko1 commented Nov 30, 2020

Ah, I have no opinion, thank you so much to provide your efforts!

@nobu nobu merged commit 6bef494 into ruby:master Nov 30, 2020
@casperisfine
Copy link
Contributor Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants