Skip to content
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

Cleaning up generating macros in _cursesmodule.c #125354

Closed
wants to merge 5 commits into from

Conversation

picnixz
Copy link
Contributor

@picnixz picnixz commented Oct 12, 2024

This is mostly a cosmetic but useful PR. Why? because it's for future maintenance. For now, all generated methods have the following signature (modulo the Py_UNUSED):

static PyObject *NAME(PyCursesWindowObject *self, PyObject *Py_UNUSED(args));

and they are stored in PyCursesWindow_methods using (PyCFunction) casts. I'm not sure whether we want to keep an explicit cast here or not but since we are also fixing UBSan failures, I think this one would count as one.

Instead of changing it now, I first suggest cleaning up the macros and then, once we also fixed Argument Clinic generation, we may then fix the UBSan failures here as well (or I can patch half of the Window's object methods since the other half is AC-generated).

I don't see when we'll need to call a method directly using its implementation and not via thePyObject_Call* API so we can also patch the macros directly in this PR.

@vstinner please tell me whether you want me to include the UBSan failure fix in this PR or not (for that reason, I will not put an issue number yet since I don't know whether it should be part of #123961 or #111178).

@picnixz picnixz force-pushed the curses/macro-cleanup-123961 branch from adb6a33 to 6f84b18 Compare October 13, 2024 12:40
@picnixz
Copy link
Contributor Author

picnixz commented Oct 13, 2024

The commit 6f84b18 is only for removing the stateful part in the names of the functions, making most of the functions named with a shorter name. There are only 3-4 occurrences and usage of a stateless check but many occurrences of a stateful one.

If you want a separate PR, I can just cherry-pick that commit though it's easier to include it in this one to reduce the number of whitespace changes.

@vstinner
Copy link
Member

I don't see the point of these changes, they don't solve any problem. They only look like coding style changes: -1 for me.

@picnixz
Copy link
Contributor Author

picnixz commented Oct 14, 2024

Oh I should have given more context. I wanted to add the possibility to include the Python function name and the curses function name that were involved when we raise an exception. For now, it's a generic message saying that function X raised something, but the names may not necessarily match the C function that was actually called.

What I wanted to do is essentially mimic OSError and its filename/filename2 attributes (that could help instead of parsing the message that we could change in the future). So I wanted first to cleanup the base code before continuing the work.


If you think that the error feature is not needed, I'll just close this PR and the refactorization issue as well.

@picnixz
Copy link
Contributor Author

picnixz commented Oct 22, 2024

Closing in favor of #125844.

@picnixz picnixz closed this Oct 22, 2024
@picnixz picnixz deleted the curses/macro-cleanup-123961 branch October 22, 2024 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants