-
Notifications
You must be signed in to change notification settings - Fork 748
Move code to subdirectories and rename or split up #1665
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
Conversation
@@ -609,7 +609,6 @@ internal static PyType AllocateTypeObject(string name, PyType metatype) | |||
/// </summary> | |||
static void InheritSubstructs(IntPtr type) | |||
{ | |||
#warning dead code? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some explanation of what this code does would be useful.
If you don't like the warning - figure it out! :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you think that it's dead? The function is being called, after all...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@filmor it might be doing something that is no longer needed and is essentially no-op
I guess 'dead' might not be the best term here, but from the looks of the code I can't determine its intent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll give it a try understanding it, but this warning is currently extremely verbose for something that is at least not really a problem :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@filmor see you after returning from the rabbit hole! 😂 (P.S. I never tried this particular one yet)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, this one tracks through until the initial commit, 15 years ago :) @brianlloyd, you don't happen to know what exactly you were trying to achieve here? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is required for things like PyNumber_Check
to work, see e.g. https://github.com/python/cpython/blob/31ff96712e8f89ac1056c2da880b44650002219f/Objects/abstract.c#L830-L837
I don't think it is necessarily required, since our types are structured exactly like PyHeapTypeObject
, but this can be revisited if we ever restructure our generated types to be created completely using Python's methods instead of this hackery. It's definitely valid to do this explicit initialisation here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems wrong to have it present for types that do not implement numbers protocol.
From the code you linked it looks like our types would pass - no it does not, as PyNumber_Check
.nb_index
, nb_float
or nb_int
are not set in this function.
Also, what is the relationship between sq_length
and tp_as_sequence
and why does one get set to another? And the same for the other two fields...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sq_length
etc. are the first fields of the respective abstract types. If your type /could/ implement one of them, they have to be set. And our types (being modelled in memory after heap types) always have the respective sections, so it's correct to set them. The checks pass as soon as one of the slots is filled.
1f045de
to
f9f08a0
Compare
039e49b
to
c294582
Compare
What does this implement/fix? Explain your changes.
...
Does this close any currently open issues?
...
Any other comments?
...
Checklist
Check all those that are applicable and complete.
AUTHORS
CHANGELOG