Update argc type from size_t to int

Frederick888 commented 1 year ago

`4c6ed4f` fix: Update argc type from size_t to int

There was some rework in Ruby around ANYARGS and it now doesn't compile with Ruby 2.7.8 and 3.2.2 on macOS due to signature mismatch.

Under Linux this is only a warning:

/usr/local/include/ruby-3.2.0/ruby/internal/anyargs.h:308:143: warning: passing argument 3 of ‘rb_define_singleton_method_m1’ from incompatible pointer type [-Wincompatible-pointer-types]
  308 | #define rb_define_singleton_method(obj, mid, func, arity)   RBIMPL_ANYARGS_DISPATCH_rb_define_singleton_method((arity), (func))((obj), (mid), (func), (arity))
      |                                                                                                                                               ^~~~~~
      |                                                                                                                                               |
      |                                                                                                                                               VALUE (*)(size_t,  VALUE *, VALUE) {aka long unsigned int (*)(long unsigned int,  long unsigned int *, long unsigned int)}

tonytonyjan commented 1 year ago

hi @Frederick888, thanks for sending the PR. I recently replaced CI with github workflow and it looks like tests are all passed with mac v12\~v13 + ruby v2.7\~v3.2.

https://github.com/tonytonyjan/jaro_winkler/actions/runs/4955486475

To proceed this PR, can you also try to change CI configuration or provide instructions so that others can reproduce the error?

Frederick888 commented 1 year ago

@tonytonyjan Sure, let me try. Could you approve the workflow run?

tonytonyjan commented 1 year ago

@Frederick888 I have approved the workflow run. You should be able to run workflow once you update the branch of this PR. Let me know if it doesn't work, thx!

Frederick888 commented 1 year ago

@tonytonyjan Failed run. Successful one.

I'm not sure why incompatible-pointer-types is an error sometimes on macOS. Maybe it's because I used LLVM from Homebrew? I'm on Linux right now but I can look into it next week if needed.

And by the way I made an additional small change to Actions manifest 1875fa6ecd6eb708cb36b14773ecd11e12fcd935. Let me know if you want me to get rid of it.

tonytonyjan commented 1 year ago

Hi @Frederick888 sorry for responding so late...

After some investigation, I'll merge this PR if you can finish the following tasks:

discard the changes from .github/workflows/test.yml, ext/jaro_winkler/extconf.rb.
squash commits into 1 single commit.

I verified the change ext/jaro_winkler/jaro_winkler.c can remove warning messages and works expectedly. Though we don't know the root cause yet but I think it's good to merge. :)

I prefer to make PRs small and focusing, thus .github/workflows/test.yml should be another PR.

Thank you for your contribution and let me know if you have any concern. 🍻

Frederick888 commented 1 year ago

After some investigation, I'll merge this PR if you can finish the following tasks:

discard the changes from |.github/workflows/test.yml|, |ext/jaro_winkler/extconf.rb|.

squash commits into 1 single commit.

[...]

I prefer to make PRs small and focusing, thus |.github/workflows/test.yml| should be another PR.

Sure thing and done! Basically leaving just the third commit. I keep the commits atomic and self-contained exactly for this reason :)

On 14/5/23 14:22, 簡煒航 (Weihang Jian) wrote:

Hi @Frederick888 https://github.com/Frederick888 sorry for responding so late...

After some investigation, I'll merge this PR if you can finish the following tasks:

discard the changes from |.github/workflows/test.yml|, |ext/jaro_winkler/extconf.rb|.

squash commits into 1 single commit.

I verified the change |ext/jaro_winkler/jaro_winkler.c| can remove warning messages and works expectedly. Though we don't know the root cause yet but I think it's good to merge. :)

I prefer to make PRs small and focusing, thus |.github/workflows/test.yml| should be another PR.

Thank you for your contribution and let me know if you have any concern. 🍻

— Reply to this email directly, view it on GitHub https://github.com/tonytonyjan/jaro_winkler/pull/52#issuecomment-1546803487, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABCMP7YWP5CCRVDRHVZRD3LXGBMZDANCNFSM6AAAAAAX4CHZEU. You are receiving this because you were mentioned.Message ID: @.***>

-- Frederick Zhang

PGP: 8BFB EA5B 4C44 BFAC C8EC 5F93 1F92 8BE6 0D8B C11D

tonytonyjan commented 1 year ago

@Frederick888

I appreciate your contribution, thanks for making this gem better! 🎉

Frederick888 commented 1 year ago

I think I know why this happened. It was LLVM from Homebrew like I suspected.

https://discourse.llvm.org/t/clang-16-notice-of-potentially-breaking-changes/65562

-Wincompatible-function-pointer-types now defaults to an error in all C language modes.

I switched to system's built-in clang 14 and was able to compile v1.5.4, where incompatible-function-pointer-types was warning only.

tonytonyjan / jaro_winkler