Skip to content

Don't support blockarg in opt_new #13198

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
Merged
merged 2 commits into from
Apr 29, 2025
Merged

Don't support blockarg in opt_new #13198

merged 2 commits into from
Apr 29, 2025
+51 −2

Conversation

tekknolagi
Copy link
Contributor

@tekknolagi tekknolagi commented Apr 28, 2025 •
edited
Loading

Pairin' with Aaron.

cc @tenderlove

@tekknolagi
Don't support blockarg in opt_new
9b47288 
We don't calculate the correct argc so the bookkeeping slot is something
else (unexpected) instead of Qnil (expected).
@tekknolagi tekknolagi marked this pull request as ready for review April 29, 2025 03:26
@tekknolagi
Copy link
Contributor Author

I would like to add a bunch of tests. Where should they go?

@tekknolagi
Copy link
Contributor Author

Linking to #13080

@tekknolagi
Copy link
Contributor Author

This testing code feels extremely ugly so if you have tips for how to improve, please let me know.

@launchable-app Launchable app

This comment has been minimized.

Sign in to view
@tekknolagi tekknolagi force-pushed the mb-no-blockarg branch 2 times, most recently from 062f651 to 3401057 Compare April 29, 2025 13:17
@tenderlove
Copy link
Member

This testing code feels extremely ugly so if you have tips for how to improve, please let me know.

Were you able to make a short repro that causes a segv with RUBY_DEBUG enabled? If so, I think that would be a good enough test. I think we have a RUBY_DEBUG build so it should catch it. The tests you added are fine, but they seem a little brittle because I think we could support opt_new in some of those cases, it will just take work. That said, I'm fine merging this.

@tekknolagi
Copy link
Contributor Author

I think it's fine to have a test whose output we expect to change when we add an additional feature. We can migrate from the failing section to the passing section.

I got these to assert fail as expected (not segv), yes

@tenderlove tenderlove merged commit 10fd5a6 into ruby:master Apr 29, 2025
82 of 85 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tekknolagi @tenderlove