Warn unless T_DATA object classes redefine or undefine the alloc function#4604
Merged
nobu merged 2 commits intoruby:masterfrom Aug 19, 2021
Merged
Warn unless T_DATA object classes redefine or undefine the alloc function#4604nobu merged 2 commits intoruby:masterfrom
nobu merged 2 commits intoruby:masterfrom
Conversation
Contributor
Author
|
Backlink to https://bugs.ruby-lang.org/issues/18007 |
43475f2 to
5728e5f
Compare
allocate
Contributor
Author
|
Updated in response to Nobu's feedback at https://bugs.ruby-lang.org/issues/18007#note-1 |
nobu
reviewed
Jul 11, 2021
5728e5f to
f9e6e92
Compare
Contributor
Author
|
@nobu I've made the requested change, squashed and rebased. Thank you again for your time. |
Member
|
Great! |
f9e6e92 to
22de9b0
Compare
Contributor
Author
|
I've rebased onto |
nobu
reviewed
Aug 19, 2021
e2853e9 to
15338e7
Compare
which have not undefined or redefined it. When a `T_DATA` object is created whose class has not undefined or redefined the alloc function, the alloc function now gets undefined by Data_Wrap_Struct et al. Optionally, a future release may also warn that this being done. This should help developers of C extensions to meet the requirements explained in "doc/extension.rdoc". Without a check like this, there is no easy way for an author of a C extension to see where they have made a mistake.
15338e7 to
e50d558
Compare
allocateper guidance in doc/extension.rdoc, these classes now undefine their alloc functions: - ObjectSpace::InternalObjectWrapper - Socket::Ifaddr
e50d558 to
bddaecf
Compare
This was referenced May 17, 2022
konsolebox
added a commit
to konsolebox/digest-kangarootwelve-ruby
that referenced
this pull request
Jul 22, 2023
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Backlink to https://bugs.ruby-lang.org/issues/18007
Problem being solved
This option is intended to help developers of C extensions to check if their code meets the requirements explained in "doc/extension.rdoc". Specifically, I want to ensure that
T_DATAobject classes undefine or redefine the alloc function.There is currently no easy way for an author of a C extension to easily see where they have made the mistake of letting the class's default alloc function remain.
Description of the solution
Ruby will undefine the alloc func when a
T_DATAobject is created whose class has not undefined or redefined the alloc function.A new function is defined,
rb_data_object_check. That function is called fromrb_data_object_wrap()andrb_data_typed_object_wrap()(which implement theData_Wrap_Structfamily of macros).An optional warning can be enabled which, when emitted, looks like this:
Examples of this problem in the wild
Using this code, I found that many of Nokogiri's classes needed to undefine their alloc funcs.
This PR also updates these core Ruby classes by undefining their alloc func:
ObjectSpace::InternalObjectWrapperSocket::IfaddrQuestions for reviewers
Should this warning be emitted with the
deprecatedcategory?Benchmarking
I benchmarked this code by allocating
Nokogiri::XML::NodeSets in a loop. This is a class with a relatively simple alloc function.The runs cover the four combinations of enabled/disabled, and warnings/no-warnings.