search

blackwinter / ruby-filemagic

Ruby bindings to the magic(4) library, revised.
https://blackwinter.github.iom/ruby-filemagic
146 stars 34 forks source link

Undefine allocation function for C extension class #46

Open stoivo opened 1 year ago

stoivo commented 1 year ago

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

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

Other MR doing similar change

https://github.com/vmg/redcarpet/pull/721 https://github.com/appsignal/appsignal-ruby/pull/917

kevingriffin commented 1 year ago

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 commented 1 year ago

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 commented 10 months ago

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

namely-sachin commented 8 months ago

I am facing similar issue.

namely-sachin commented 8 months ago

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

stoivo commented 6 months ago

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

Lykos commented 6 months ago

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!