-
Notifications
You must be signed in to change notification settings - Fork 85
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
More comprehensive recursion guard. #460
Conversation
self.ec = ec | ||
if w_func_id not in self.ec.recursive_calls: | ||
self.ec.recursive_calls[w_func_id] = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be using the underlying strings, not the symbols.
if !x.eql?(other[i]) | ||
return false | ||
Thread.current.recursion_guard(:array_eqlp, self) do |recursion| | ||
if !recursion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the change in API from returning the the in_recursion
value to yielding it to the block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran into a case where not letting the block decide what to do when recursion is detected made things difficult, but I'm not sure if that's an issue anymore.
Sorry, I'm still thinking this over :/ |
end | ||
end | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't used yet, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, but I need it for Array#hash in #456.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is becoming my official theme song, but let's split this up :D, so this just has the identifier
for recursion guards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it have Thread.in_recursion_guard?
as well? That's only used by the outer guard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, just what's needed. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should have been more precise, just what's needed for the recursion_guard()
changes.
Conflicts: tests/objects/test_threadobject.py topaz/objects/threadobject.py
This branch is very messy with all the merging and whatnot that's happened. If you prefer, I can make a clean one and just commit the final diff to it. |
This changes the behaviour of
Thread#recursion_guard
and addsThread#recursion_guard_outer
in order to allow more flexibility in how recursion is handled in Ruby code. In addition, recursion detection is now based on a function identifier rather being global to prevent nested guards from different functions interfering with each other.