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

Delete ANYARGS #2404

Closed
wants to merge 18 commits into from
Closed

Delete ANYARGS #2404

wants to merge 18 commits into from

Conversation

@shyouhei
Copy link
Member

shyouhei commented Aug 27, 2019

OK, I understand it's very difficult to implement rb_define_method without ANYARGS. But how about rb_rescue ? Because that function has only one usage, I see no reason for its function prototype to use ANYARGS. And here is my changeset to delete such usages.

shyouhei added 18 commits Aug 26, 2019
After 5e86b00, I now think ANYARGS is
dangerous and should be extinct.  Let's start from making
rb_block_call_func_t strict, and apply RB_BLOCK_CALL_FUNC_ARGLIST liberally.
After 5e86b00, I now think ANYARGS is
dangerous and should be extinct.  This commit deletes ANYARGS from
struct vm_ifunc, but in doing so we also have to decouple the usage
of this struct in compile.c, which (I think) is an abuse of ANYARGS.
After 5e86b00, I now think ANYARGS is
dangerous and should be extinct.  This commit makes rb_iterate free
from ANYARGS.
After 5e86b00, I now think ANYARGS is
dangerous and should be extinct.  This commit deletes ANYARGS from
rb_rescue / rb_rescue2, which revealed many arity / type mismatches.
After 5e86b00, I now think ANYARGS is
dangerous and should be extinct.  This commit deletes ANYARGS from
rb_ensure, which also revealed many arity / type mismatches.
After 5e86b00, I now think ANYARGS is
dangerous and should be extinct.  This commit deletes ANYARGS from
rb_catch, and fixes some bugs revealed by that.
After 5e86b00, I now think ANYARGS is
dangerous and should be extinct.  This commit deletes ANYARGS from
rb_proc_new / rb_fiber_new, and applies RB_BLOCK_CALL_FUNC_ARGLIST
wherever necessary.
After 5e86b00, I now think ANYARGS is
dangerous and should be extinct.  This commit deletes ANYARGS from
rb_thread_create, which seems very safe to do.
After 5e86b00, I now think ANYARGS is
dangerous and should be extinct.  This commit deletes ANYARGS from
st_foreach.  I strongly believe that this commit should have had come
with b0af059, which added extra
parameter to st_foreach callbacks.
After 5e86b00, I now think ANYARGS is
dangerous and should be extinct.  This commit uses rb_gvar_getter_t /
rb_gvar_setter_t for rb_define_hooked_variable /
rb_define_virtual_variable which revealed lots of function prototype
inconsistencies.  Some of them were literally decades old, going back
to dda5dc0.
After 5e86b00, I now think ANYARGS is
dangerous and should be extinct.  This commit adds function prototypes
for rb_hash_foreach / st_foreach_safe.  Also fixes some prototype
mismatches.
After 5e86b00, I now think ANYARGS is
dangerous and should be extinct.  This commit adds a function
prototype for rb_ivar_foreach.  Luckily this change revealed no
problematic usage of the function.
After 5e86b00, I now think ANYARGS is
dangerous and should be extinct.  This commit adds function prototypes
for struct st_hash_type.  Honestly I don't understand why they were
commented out at the first place.
After 5e86b00, I now think ANYARGS is
dangerous and should be extinct.  There is only one usage of
MEMO::u3::func in load.c (where void Init_Foobar(vodi) is registered)
so why not just be explicit.
After 5e86b00, I now think ANYARGS is
dangerous and should be extinct.  This function has only one call site
so adding appropriate prototype is trivial.
They lack portability. See also
https://travis-ci.org/shyouhei/ruby/jobs/577164015
@shyouhei

This comment has been minimized.

Copy link
Member Author

shyouhei commented Aug 27, 2019

Will merge this.

@shyouhei

This comment has been minimized.

Copy link
Member Author

shyouhei commented Aug 27, 2019

done @ 1663d34

@shyouhei shyouhei closed this Aug 27, 2019
matzbot pushed a commit that referenced this pull request Aug 27, 2019
in response to the declaration change in
e3fc305.

Fixing the AppVeyor mswin CI failure:
https://ci.appveyor.com/project/ruby/ruby/builds/26980881/job/2j6h1qwjnbc8cpop

ref: #2404
@k0kubun k0kubun mentioned this pull request Sep 1, 2019
matzbot pushed a commit that referenced this pull request Sep 6, 2019
Compilation of extension libraries written in C++ are reportedly
broken due to #2404

The root cause of this issue was that the definition of ANYARGS
differ between C and C++, and that of C++ is incompatible with the
updated ones.

We are using the incompatibility against itself.  In C++ two distinct
function prototypes can be overloaded.  We provide the old, ANYARGSed
prototypes in addition to the current granular ones; and let the
older ones warn about types.
@sodabrew

This comment has been minimized.

Copy link

sodabrew commented Sep 9, 2019

Since a569bc0 adds back a compatibility layer for C++, what should C++ extensions do as a best practice going forward?

@shyouhei

This comment has been minimized.

Copy link
Member Author

shyouhei commented Sep 9, 2019

@sodabrew Currently, if you are using C++ and stick to old style, your compiler complains about it but compiles nonetheless. I suggest there are two possible best practices:

  • If you value static type cheking than backwards compatibility: follow the compiler warnings. That can save you from e.g. arity mismatch.
  • If you value backwards compatibility than that: just ignore what the compiler complains. Programs worked before should work as-is I believe. If not please file a bug report.
@shyouhei shyouhei deleted the shyouhei:ANYARGS branch Sep 12, 2019
@larskanis

This comment has been minimized.

Copy link
Contributor

larskanis commented Sep 27, 2019

@shyouhei Just to give you a note: FXRuby failed to build due to latest changes to ANYARGS and C++ type checking/depreciation. I had to do a bunch of changes to make it to compile again and not raising any warnings. So the ANYARGS changes in ruby break backward compatibility in some cases.

However I'm glad to see these changes! I always wondered, why ruby's C-API makes overly much use of ANYARGS. It also revieled some broken function signatures in FXRuby.

eregon added a commit to eregon/rubyspec that referenced this pull request Sep 29, 2019
in response to the declaration change in
e3fc30564e9466d6926f9d25a090dcf787bd5c33.

Fixing the AppVeyor mswin CI failure:
https://ci.appveyor.com/project/ruby/ruby/builds/26980881/job/2j6h1qwjnbc8cpop

ref: ruby/ruby#2404
@shyouhei

This comment has been minimized.

Copy link
Member Author

shyouhei commented Oct 1, 2019

@larskanis Thank you! The build failure seems related to 17a1366. @nobu Can you take a look?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.