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

rb_gc_force_recycle is now no-op, Ruby 3.1 support #2908

Merged

Conversation

moste00
Copy link
Contributor

@moste00 moste00 commented Mar 6, 2023

Removed the body of rb_gc_force_recycle, a C API function that #2733 says that is deprecated and should be made a no-op. Full text search confirms it's not used anywhere.

@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Mar 6, 2023
@andrykonchin
Copy link
Member

andrykonchin commented Mar 7, 2023

Linter fails with error:

Check the diff of this PR, and:
* if ABI has changed, then increment lib/cext/ABI_version.txt
* if ABI has not changed, then increment lib/cext/ABI_check.txt

Changing headers or compilation flags, as well as removing/adding a non-static function
(because e.g. mkmf have_func can depend on that) should all be considered ABI changes.

So one more change is required. Looks like ABI wasn't changed so could you please increment lib/cext/ABI_check.txt (it should probably be done in the same commit with changes in a header so satisfy the linter check)

@eregon
Copy link
Member

eregon commented Mar 7, 2023

Full text search

Do you mean GitHub search? I wouldn't trust that, rather git grep or so is reliable.

@eregon
Copy link
Member

eregon commented Mar 7, 2023

Upstream is https://github.com/ruby/ruby/blob/ruby_3_1/gc.c#L8675-L8679
Could you copy-paste that function declaration so we have exactly the same?

It's also deprecated here https://github.com/ruby/ruby/blob/ruby_3_1/include/ruby/internal/intern/gc.h#L213-L214
but we already have that:

RBIMPL_ATTR_DEPRECATED(("this is now a no-op function"))
so that part is already done.

@moste00
Copy link
Contributor Author

moste00 commented Mar 7, 2023

Do you mean GitHub search?

Oh I meant Intellij IDEA, I did "find in files" and it found nothing except the .h and .c where it's defined. I did git grep just now and it agrees.

Could you copy-paste that function declaration so we have exactly the same?

Hmmm, is there an automated check for that which we would like to pass ? It's the same exact function with a different formatting, I will do it anyway but I don't understand why we should string-match the exact file in CRuby :'D.

@moste00 moste00 force-pushed the rb_gc_force_recycle_Deprecated_NoOp branch from 92a76ba to a481c03 Compare March 7, 2023 18:58
@moste00
Copy link
Contributor Author

moste00 commented Mar 7, 2023

So one more change is required.

Done now.

@eregon
Copy link
Member

eregon commented Mar 8, 2023

Hmmm, is there an automated check for that which we would like to pass ? It's the same exact function with a different formatting, I will do it anyway but I don't understand why we should string-match the exact file in CRuby :'D.

Yeah we don't have to match exactly, e.g. we can respect the style of this file with 2-spaces indent and return type and { on the same line, etc. But it's good to look very close to the original so it's easy to compare with a glance.
Notably here we wouldn't want to keep the outdated comment.

@eregon eregon self-assigned this Mar 8, 2023
@eregon eregon added this to the 23.0.0 Release (April 18, 2023) milestone Mar 8, 2023
@eregon eregon added the in-ci The PR is being tested in CI. Do not push new commits. label Mar 8, 2023
@eregon eregon force-pushed the rb_gc_force_recycle_Deprecated_NoOp branch from a481c03 to 8dca059 Compare March 8, 2023 13:57
@andrykonchin andrykonchin force-pushed the rb_gc_force_recycle_Deprecated_NoOp branch from 8dca059 to 3735ce4 Compare March 14, 2023 18:06
@andrykonchin andrykonchin force-pushed the rb_gc_force_recycle_Deprecated_NoOp branch from 3735ce4 to c4507e5 Compare March 14, 2023 18:06
@graalvmbot graalvmbot merged commit cd38a32 into oracle:master Mar 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in-ci The PR is being tested in CI. Do not push new commits. OCA Verified All contributors have signed the Oracle Contributor Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants