Skip to content

GH-133372: C implementation of types._GeneratorWrapper is added. #133373

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

Closed

Conversation

efimov-mikhail
Copy link
Contributor

@efimov-mikhail efimov-mikhail commented May 4, 2025

@efimov-mikhail efimov-mikhail marked this pull request as ready for review May 4, 2025 13:09
@ZeroIntensity ZeroIntensity requested a review from AA-Turner May 4, 2025 14:18
Copy link
Member

@ZeroIntensity ZeroIntensity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this should go in the generator object implementation. _types should implement it on its own.

Py_CLEAR(gw->gw_gen);
Py_CLEAR(gw->gw_name);
Py_CLEAR(gw->gw_qualname);
if (gw->gw_weakreflist != NULL)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per PEP 7, all if clauses are supposed to have braces.

"__name__ must be set to a string object");
return -1;
}
Py_XSETREF(gw->gw_name, Py_NewRef(value));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need a critical section if we're going to modify this in methods.

Copy link
Contributor Author

@efimov-mikhail efimov-mikhail May 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please give me some additional context on it?
I've just copied this code from gen_set_name function.
Does this mean that critical section is also needed in that func?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@efimov-mikhail
Copy link
Contributor Author

I close this because of #134563

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