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

Add built-in ObjectSpace definition #477

Merged
merged 1 commit into from
Nov 30, 2020
Merged

Add built-in ObjectSpace definition #477

merged 1 commit into from
Nov 30, 2020

Conversation

ybiquitous
Copy link
Contributor

This change adds the built-in ObjectSpace module definition:
https://ruby-doc.org/core-2.7.2/ObjectSpace.html

Note that this change does not include the following definitions to keep the changeset small:


private

def garbage_collect: (?full_mark: bool, ?immediate_mark: bool, ?immediate_sweep: bool) -> void
Copy link
Contributor Author

Choose a reason for hiding this comment

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

[note] I don't know how to test this private method, so the test is not added.

ObjectSpace, :each_object, klass
# NOTE: Commented out because it's too slow.
# assert_send_type "() { (top) -> void } -> Integer",
# ObjectSpace, :each_object do |obj| obj.to_s end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

[note] If running this commented-out test, the following failure occur:

ObjectSpaceTest#test_each_object:
ArgumentError: string contains null byte
    /Users/masafumi.koba/git/ybiquitous/rbs/test/stdlib/test_helper.rb:243:in `inspect'
    /Users/masafumi.koba/git/ybiquitous/rbs/test/stdlib/test_helper.rb:243:in `inspect'
    /Users/masafumi.koba/git/ybiquitous/rbs/test/stdlib/test_helper.rb:243:in `inspect'
    /Users/masafumi.koba/git/ybiquitous/rbs/test/stdlib/test_helper.rb:243:in `assert_send_type'
    /Users/masafumi.koba/git/ybiquitous/rbs/test/stdlib/objectspace_test.rb:38:in `test_each_object'

@ybiquitous ybiquitous marked this pull request as ready for review November 16, 2020 15:31
@ybiquitous
Copy link
Contributor Author

I think this PR is ready to review, but some tests are disabled still due to they are too slow.
Could you please tell me if you have some good ideas?

# Total count: 7
#
def self.each_object: (?Module `module`) -> Enumerator[top, Integer]
| (?Module `module`) { (top obj) -> void } -> Integer
Copy link
Member

Choose a reason for hiding this comment

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

@ybiquitous Do you want to make the type of obj top? It looks like untyped is fine to me...

Copy link
Contributor Author

@ybiquitous ybiquitous Nov 24, 2020

Choose a reason for hiding this comment

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

I have no opinion here. Should I fix also top at other points in this file?
(I'll follow your suggestion)

Copy link
Member

Choose a reason for hiding this comment

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

Ok. Please make it untyped. 🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed via b3f2590

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll rebase the branch after approval.

Copy link
Member

@soutaro soutaro left a comment

Choose a reason for hiding this comment

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

@ybiquitous 👍 👏

@ybiquitous
Copy link
Contributor Author

@soutaro Thank you for the review! I've squashed to one commit.

@soutaro soutaro merged commit fccfab9 into ruby:master Nov 30, 2020
@ybiquitous ybiquitous deleted the add-object-space branch December 1, 2020 00:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants