Skip to content

fix Array#delete always firing the block when deleting nil - #6235

Merged
matz merged 5 commits into
mruby:masterfrom
phi-luna:array/delete
Apr 15, 2024
Merged

matz merged 5 commits into
mruby:masterfrom
phi-luna:array/delete

Conversation

@phi-luna

Copy link
Copy Markdown
Contributor

reimplement Array#delete in c, fixing ary.delete(nil, &blk) firing the block regardless of removal

minimal reproduction:

ary = [nil]
ret = ary.delete(nil) { "not deleted?" }

result on master:

ret
# => "not deleted?"
ary
# => []

result after this patch:

ret 
# => nil
ary
# => []

reimplement `Array#delete` in c, fixing `ary.delete(nil, &blk)` firing the block regardless of removal

minimal reproduction:
```rb
ary = [nil]
ret = ary.delete(nil) { "not deleted?" }
```
@phi-luna
phi-luna requested a review from matz as a code owner April 12, 2024 15:33
@github-actions github-actions Bot added the core label Apr 12, 2024
@phi-luna
phi-luna marked this pull request as draft April 12, 2024 15:36
@phi-luna
phi-luna marked this pull request as ready for review April 12, 2024 15:51
@matz

matz commented Apr 13, 2024

Copy link
Copy Markdown
Member

Thanks for the PR. But as a principle, the mruby standard library does not call blocks from C code unless absolutely necessary (to avoid VM recursion). I do not consider it an absolute necessity this time.
We have several options:

  • After PR merge, I will modify it not to call the block
  • I fix the Ruby code
  • @leviongit will update the PR

Which is your preference?

@phi-luna

Copy link
Copy Markdown
Contributor Author

thank you for the comment. i will amend the pr.

@matz

matz commented Apr 13, 2024

Copy link
Copy Markdown
Member

I that case, you can implement the body part in C (which probably returns index or - 1; you may name it __delete), or you can fix the Ruby code.

@phi-luna

phi-luna commented Apr 13, 2024 •

Copy link
Copy Markdown
Contributor Author

i will check whether implementing the core part in c is worthwhile and will come back with results

edit:
i believe it is worthwhile (and it was fairly easy to do so)

@phi-luna

Copy link
Copy Markdown
Contributor Author

this pr is ready to merge

@matz
matz merged commit c01899a into mruby:master Apr 15, 2024
@matz

matz commented Apr 16, 2024

Copy link
Copy Markdown
Member

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants