Skip to content

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

Merged
merged 2 commits into from
Jan 9, 2022

Conversation

filmor
Copy link
Member

@filmor filmor commented Jan 8, 2022

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.

  • Make sure to include one or more tests for your change
  • If an enhancement PR, please create docs and at best an example
  • Add yourself to AUTHORS
  • Updated the CHANGELOG

@@ -609,7 +609,6 @@ internal static PyType AllocateTypeObject(string name, PyType metatype)
/// </summary>
static void InheritSubstructs(IntPtr type)
{
#warning dead code?
Copy link
Member

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! :)

Copy link
Member Author

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...

Copy link
Member

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.

Copy link
Member Author

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 :)

Copy link
Member

@lostmsu lostmsu Jan 8, 2022

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)

Copy link
Member Author

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? :)

Copy link
Member Author

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.

Copy link
Member

@lostmsu lostmsu Jan 9, 2022

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 PyNumber_Check. - no it does not, as 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...

Copy link
Member Author

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.

@filmor filmor force-pushed the move-everything-around branch from 1f045de to f9f08a0 Compare January 8, 2022 16:09
@filmor filmor requested a review from lostmsu January 8, 2022 21:31
@filmor filmor force-pushed the move-everything-around branch from 039e49b to c294582 Compare January 9, 2022 20:01
@filmor filmor merged commit 67dded2 into pythonnet:master Jan 9, 2022
@filmor filmor deleted the move-everything-around branch January 9, 2022 20:03
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.

2 participants