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

Undefine allocation function for C extension class #46

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

stoivo
Copy link

@stoivo stoivo commented Apr 25, 2023

  • Undefine allocation function for C extension class

Since Ruby 3.2 a new warning is printed when a Ruby class created in a C extension does not specify an allocate function or undefine it.

warning: undefining the allocator of T_DATA class FileMagic (WarningHandlers::Ruby::Warning)

From my understanding, we only need to define an allocate function if the class uses a C struct and stores any values on it. Our classes don't do that in C, that's done in our Rust extension.

Closes #909

Resources

https://bugs.ruby-lang.org/issues/18007
https://github.com/ruby/ruby/blob/6963f8f743b42f9004a0879cd66c550f18987352/doc/extension.rdoc#label-Write+the+C+Code https://ruby-doc.org/core-3.1.1/doc/extension_rdoc.html#label-C+struct+to+Ruby+object

rails-sqlserver/tiny_tds#515 https://groups.google.com/g/sequel-talk/c/K0J80s4wGJU/m/BT-6FFhrAgAJ

Other MR doing similar change

vmg/redcarpet#721
appsignal/appsignal-ruby#917

@kevingriffin
Copy link

Hi! I had a look at this for our usecase. It seems valid for the FileMagic class, as it shouldn't be allocated, but FileMagicError does get used like a regular class. I see these two cases:

  if ((ms = magic_open(NUM2INT(args[0]))) == NULL) {
    rb_raise(rb_FileMagicError,
      "failed to initialize magic cookie (%d)", errno || -1);
  }

  if (magic_load(ms, NULL) == -1) {
    rb_raise(rb_FileMagicError,
      "failed to load database: %s", magic_error(ms));
  }

in which case, I'm not sure this change is valid. Have you tested these error cases?

@stoivo
Copy link
Author

stoivo commented Jun 27, 2023

No, I have haven't tested it. I don't know how to either. My knowledge of C is low so Im not sure what I can do to test it

@bforma
Copy link

bforma commented Oct 16, 2023

Any update on this? I've tried this and it works for us. The warning is no longer shown.

@namely-sachin
Copy link

I am facing similar issue.

@namely-sachin
Copy link

Please merge this PR and release a new version of gem.

* Undefine allocation function for C extension class

Since Ruby 3.2 a new warning is printed when a Ruby class created in a C
extension does not specify an allocate function or undefine it.

```
/gem/lib/appsignal/transaction.rb:98: warning: undefining the allocator of T_DATA class Appsignal::Extension::Transaction
/gem/lib/appsignal/utils/data.rb:19: warning: undefining the allocator of T_DATA class Appsignal::Extension::Data
```

From my understanding, we only need to define an allocate function if
the class uses a C struct and stores any values on it. Our classes don't
do that in C, that's done in our Rust extension.

Closes #909

## Resources

https://bugs.ruby-lang.org/issues/18007
https://github.com/ruby/ruby/blob/6963f8f743b42f9004a0879cd66c550f18987352/doc/extension.rdoc#label-Write+the+C+Code
https://ruby-doc.org/core-3.1.1/doc/extension_rdoc.html#label-C+struct+to+Ruby+object

rails-sqlserver/tiny_tds#515
https://groups.google.com/g/sequel-talk/c/K0J80s4wGJU/m/BT-6FFhrAgAJ

## Other MR doing similar change
vmg/redcarpet#721
appsignal/appsignal-ruby#917
@stoivo
Copy link
Author

stoivo commented Jan 29, 2024

@kevingriffin, We did some testing on our side and we think you are right. I should remove some of my changes

@Lykos
Copy link

Lykos commented Feb 5, 2024

I just created issue #47 about this problem and now I saw this pull request fixes it. Not that I have anything to say, but I am supportive of fixing this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants