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

Handle recursion in Array#hash. #456

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jerith
Copy link
Contributor

@jerith jerith commented Mar 2, 2013

Three of the specs are still tagged. Two of these need Array#fill which isn't yet implemented and the third probably needs fixes to Hash#hash.

catch(:array_hash_recursion) do
return self.inner_hash(res)
end
return res
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand the logic here at all, at a minimum it needs an explanatory comment.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, now I understand it, using throw/catch seems way too complex, why not just use the normal recursion_guard and if the object is in a recursion return 0 or something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That catches the recursion on the inside rather than the outside, and we've already modified the hash for the intervening layers. We get a.hash == [[a]].hash because of the XOR, but a.hash != [a].hash.

@jerith
Copy link
Contributor Author

jerith commented Mar 3, 2013

Turns out we need finer-grained recursion guarding before we can do this thing safely.

@jerith
Copy link
Contributor Author

jerith commented Mar 3, 2013

I'll update this to a simpler version that uses Thread.recursion_guard_outer once #460 is in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants