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.

Sign up for GitHub

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

Jump to bottom

Fix ownership error in vformat test #4868

Closed
wants to merge 1 commit into from
Closed

Fix ownership error in vformat test #4868

wants to merge 1 commit into from

Conversation

take-cheeze
Copy link
Contributor

No description provided.

@matz
Copy link
Member

matz commented Dec 13, 2019

I don't see the reason to protect the %C class only. Does this PR fix any real problem?

@take-cheeze
Copy link
Contributor Author

This bothers me when testing with -DMRB_GC_STRESS.
And object under RData's memory don't get marked so it's potentially unsafe operation anyway.

@shuujii
Copy link
Contributor

shuujii commented Dec 14, 2019

This issue seems to be resolved by #4622. Will it not be merged?

@matz
Copy link
Member

matz commented Dec 17, 2019

I don't think %C (class/module) needs to be saved in TestVFormat::Native. %C should be handled like %t.

@matz matz closed this in f507ff4 Dec 17, 2019
@take-cheeze take-cheeze deleted the vf_ownership branch December 18, 2019 03:21
matz added a commit that referenced this pull request Dec 18, 2019
@matz
Revert "%C value need not to be saved in TestVFormat::Native; close
7f0e1fc 
#4868"

This reverts commit f507ff4.
It makes AppVeyor tests fail.
matz added a commit that referenced this pull request Dec 19, 2019
@matz
Reimplement vformat tests; close #4868
429f6de 
Avoid creating `Data` object that refers `mruby` objects.
Also close #4622 ref #4613
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@take-cheeze @matz @shuujii