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.

Sign up for GitHub

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

Jump to bottom

Eagerly defrost chilled strings #94

Merged
merged 1 commit into from
Mar 27, 2024
Merged

Eagerly defrost chilled strings #94

merged 1 commit into from
Mar 27, 2024

Conversation

etiennebarrie
Copy link
Contributor

@casperisfine casperisfine mentioned this pull request Mar 25, 2024
Closed
eregon
eregon reviewed Mar 25, 2024
View reviewed changes
ext/stringio/extconf.rb Outdated Show resolved Hide resolved
@etiennebarrie etiennebarrie force-pushed the rb_str_chilled_p branch 3 times, most recently from 5796bc4 to a175c4c Compare March 26, 2024 11:35
@etiennebarrie
Copy link
Contributor Author

I updated this to not use have_func following the feedback and changes in ruby/ruby#10355 (comment).

I also realized there were warnings in the output, so I added assertions to ensure they're here and to remove them from the test output.

@casperisfine casperisfine force-pushed the rb_str_chilled_p branch 3 times, most recently from f4f5f70 to a175c4c Compare March 26, 2024 12:01
@byroot
Copy link
Member

byroot commented Mar 26, 2024

CI is still using a ruby-head from a couple days ago: ruby 3.4.0dev (2024-03-24T15:27:00Z :detached: 5e4b4d6674), but it's passing on my machine.

@byroot byroot requested a review from nobu March 26, 2024 12:35
nobu
nobu requested changes Mar 26, 2024
View reviewed changes
ext/stringio/stringio.c Outdated Show resolved Hide resolved
ext/stringio/stringio.c Outdated Show resolved Hide resolved
@eregon
Copy link
Member

eregon commented Mar 26, 2024

CI is still using a ruby-head from a couple days ago

I scheduled a new build. The last one failed due to assertion errors (already fixed on master).

@byroot
Copy link
Member

byroot commented Mar 26, 2024

The windows build is still lagging behind by just a handful of commits, but other than that it's now all green.

kou
kou reviewed Mar 27, 2024
View reviewed changes
ext/stringio/stringio.c Outdated Show resolved Hide resolved
test/stringio/test_stringio.rb Outdated Show resolved Hide resolved
test/stringio/test_stringio.rb Show resolved Hide resolved
@etiennebarrie etiennebarrie force-pushed the rb_str_chilled_p branch 3 times, most recently from 8b6697f to e6edc8b Compare March 27, 2024 12:14
nobu
nobu reviewed Mar 27, 2024
View reviewed changes
ext/stringio/stringio.c Outdated Show resolved Hide resolved
@etiennebarrie @byroot
Eagerly defrost chilled strings
7ae21ab 
[Bug #20390]

Co-authored-by: Jean Boussier <byroot@ruby-lang.org>
@nobu nobu merged commit 17ee957 into ruby:master Mar 27, 2024
@Earlopain
Copy link

I think this can be reverted now? ruby/ruby#10849

@eregon
Copy link
Member

eregon commented May 30, 2024

Indeed, I think so and maybe rb_str_chilled_p() can even be removed?
cc @etiennebarrie @byroot @casperisfine

@casperisfine
Copy link
Contributor

rb_str_chilled_p() can even be removed?

I don't think so, because of rb_check_frozen(). Unless we stop making it a macro (which maybe we should).

But yeah, I need to look more in details how the last change impact StringIO, we may still want to trigger the warning early to be honest. Not 100% sure.

casperisfine pushed a commit to Shopify/stringio that referenced this pull request May 30, 2024
@byroot
Remove special handling of chilled strings
06ad5cf 
[Feature #20205]

Followup: ruby#94

They no longer need to be special cases. If StringIO end up
mutating a chilled string, a warning will be emitted.
@casperisfine casperisfine mentioned this pull request May 30, 2024
Merged
@eregon
Copy link
Member

eregon commented May 30, 2024

I don't think so, because of rb_check_frozen(). Unless we stop making it a macro (which maybe we should).

Right, I mean making it non-public API, for example with a rbimpl_ prefix, or indeed not declaring it at all in public headers but then that means changing rb_check_frozen() to a function.

casperisfine pushed a commit to Shopify/stringio that referenced this pull request May 30, 2024
@byroot
Remove special handling of chilled strings
d38f5e5 
[Feature #20205]

Followup: ruby#94

They no longer need to be special cases. If StringIO end up
mutating a chilled string, a warning will be emitted.
eregon pushed a commit that referenced this pull request May 30, 2024
@byroot @eregon
Remove special handling of chilled strings
dc62d65 
[Feature #20205]

Followup: #94

They no longer need to be special cases. If StringIO end up
mutating a chilled string, a warning will be emitted.
matzbot pushed a commit to ruby/ruby that referenced this pull request May 30, 2024
@byroot @matzbot
[ruby/stringio] Remove special handling of chilled strings
15501e1 
[Feature #20205]

Followup: ruby/stringio#94

They no longer need to be special cases. If StringIO end up
mutating a chilled string, a warning will be emitted.

ruby/stringio@dc62d65449
@XrXr XrXr deleted the rb_str_chilled_p branch June 4, 2024 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nobu nobu nobu requested changes

@kou kou kou left review comments

@byroot byroot byroot left review comments

@eregon eregon eregon left review comments

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@etiennebarrie @byroot @eregon @Earlopain @casperisfine @nobu @kou