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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Array/Hash/Set construction broken on TruffleRuby #734

Conversation

ivoanjo
Copy link
Contributor

@ivoanjo ivoanjo commented Jun 24, 2018

While trying to get the Persistent-馃拵 gem specs to work on TruffleRuby, I discovered that Array/Hash/Set construction was broken in several cases.

I've added test cases for the expected behaviors of Array/Hash/Set .[] and .new, and fixed a number of broken test cases on TruffleRuby (all specs are green on MRI).

There is one remaining spec in array_spec.rb that was already broken and that is non-trivial to fix, so I'll create an issue for it.

The `make_synchronized_on_rbx` method is used to intercept the `.new`
method on Array/Hash/Set but it broke whenever the `.new` was being used
with arguments.

(This was shown by the Array/Hash/Set specs that were added in the
previous commits being broken on TruffleRuby.)
Fixes several broken testcases in the Array and Hash specs.
As I'm fixing monitor initialization issues, it seems to me that the
current approach is a bit whack-a-mole so let's at least prepare a nice
error message for users to get in case we're still missing a few corner
cases.
@eregon
Copy link
Collaborator

eregon commented Jun 24, 2018

@pitr-ch Please take a look :)

@pitr-ch
Copy link
Member

pitr-ch commented Jun 25, 2018

Thanks @ivoanjo for the fix and the tests! I'll have a in depth look soon.

@pitr-ch pitr-ch added this to the 1.0.6 milestone Jun 25, 2018
@pitr-ch pitr-ch added bug A bug in the library or documentation. in progress labels Jun 25, 2018
@pitr-ch pitr-ch self-assigned this Jun 29, 2018
@pitr-ch pitr-ch removed the patch label Jun 29, 2018
* use same approach as for JRuby
* use non public TruffleRuby API for simplicity
@pitr-ch pitr-ch force-pushed the fix-array-hash-set-constructors-truffleruby branch from ab893c7 to bdcd61f Compare July 1, 2018 19:14
@pitr-ch
Copy link
Member

pitr-ch commented Jul 1, 2018

I've updated the PR with a different solution which I've already had locally. It should be simpler since it is not needed to look up all the Array and Hash methods which create new instance with a special behavior without initialize, allocate, etc.

@ivoanjo
Copy link
Contributor Author

ivoanjo commented Jul 2, 2018

Nice! I also think the previous approach was rather brittle and could easily break in newer versions.

Let me know if I can help with anything else! TruffleRuby without concurrent-ruby is like going to Disneyland on a rainy day ;)

@pitr-ch pitr-ch merged commit 3e0d64d into ruby-concurrency:master Jul 5, 2018
@ghost ghost removed the in progress label Jul 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A bug in the library or documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants