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 Array keep_if, delete_if, reject!, select! #2069

Merged
merged 2 commits into from
Jan 29, 2020
Merged

Conversation

elia
Copy link
Member

@elia elia commented Jan 24, 2020

Match CRuby behavior when exceptions are raised in the block and about
when the array is updated.

Fixes #2067

@elia elia self-assigned this Jan 24, 2020
@elia elia force-pushed the elia/fix-delete-if-keep-if branch from 226c11f to 005b28b Compare January 24, 2020 21:58
@elia elia marked this pull request as ready for review January 24, 2020 22:32
self.splice(i2, updated.length);
}

if (raised) throw raised;
Copy link
Contributor

Choose a reason for hiding this comment

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

MRI aborts iteration once the error has been thrown:

[1,2,3].keep_if { |i| p i; raise 'a' }
1
# ...backtrace
RuntimeError (a)

So I guess this check-and-throw should be somewhere above (same for other methods)

for (var i = 0, i2 = 0, length = self.length; i < length; i++) {
try {
value = Opal.yield1(block, self[i])
} catch(error) {
Copy link
Contributor

@iliabylich iliabylich Jan 24, 2020

Choose a reason for hiding this comment

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

Also I guess it may catch and re-throw Opal.breaker and Opal.returner and thus, potentially exit the outer method call

Copy link
Member Author

Choose a reason for hiding this comment

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

From what I saw they're already managed by the compiler.

Copy link
Contributor

@iliabylich iliabylich Jan 27, 2020

Choose a reason for hiding this comment

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

compiler manages only what is written in Ruby (but catch/re-throw here is in JS). maybe I'm wrong, but does it handle break and return inside keep_if? 👍 if so, but I'm afraid it swallows the initial throw Opal.returner and re-raises it once the loop is done (MRI most probably does a hard return that aborts the loop)

Also, I'm not sure if it's actually that important. I've never seen such code 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

just out of curiosity, here's what I got:

opal:elia/fix-delete-if-keep-if ⤑ bin/opal -e'def a;4.times{|n| [1,2,3].delete_if { |i| p i; return }; p n => n};end; a'          ~/C/opal
1
opal:elia/fix-delete-if-keep-if ⤑ ruby -e'def a;4.times{|n| [1,2,3].delete_if { |i| p i; return }; p n => n};end; a'              ~/C/opal
1
opal:elia/fix-delete-if-keep-if ⤑                                                                                                 ~/C/opal

Match CRuby behavior when exceptions are raised in the block and about
when the array is updated.
Should give better performance and smaller code size.
@elia elia force-pushed the elia/fix-delete-if-keep-if branch from 005b28b to 1d6d262 Compare January 27, 2020 23:00
@elia elia requested a review from iliabylich January 27, 2020 23:04
Copy link
Member Author

@elia elia left a comment

Choose a reason for hiding this comment

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

Thanks for the review, I've updated the code to stop calling the block and behave just like MRI.

Copy link
Contributor

@iliabylich iliabylich left a comment

Choose a reason for hiding this comment

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

👍

@elia elia merged commit 0e18ef7 into master Jan 29, 2020
@elia elia deleted the elia/fix-delete-if-keep-if branch January 29, 2020 00:30
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.

delete_if behaves differently from MRI
2 participants