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

chore: decimal module macro cleanup #123791

Merged
merged 3 commits into from
Sep 9, 2024

Conversation

picnixz
Copy link
Contributor

@picnixz picnixz commented Sep 6, 2024

When I looked at the refleaks in the decimal module, I observed that some macros are not protected against dangling else and that UNUSED does not match Py_UNUSED.

I didn't want to make two different PRs (though I can cherry-pick the commits since they are easy separated); nevertheless, I think the do-while protection should be included (I've already done this kind of PRs in the past).

There are some casts are redundant and one argument that was marked as unused but that was not! Some methods used 'args' but were using the METH_NOARGS (so it could be confusing to users!) flag. There is one exception being dec_copy which both uses a METH_NOARGS and METH_O flag for __copy__ and __deepcopy__ respectively. I left it as dummy but added a small comment.

This replaces the usages of the `UNUSED` macro which
was not consistent with the `Py_UNUSED` macro itself.

In addition, this amends the parameter names so that
they match their semantic meanings.
@picnixz
Copy link
Contributor Author

picnixz commented Sep 6, 2024

cc @vstinner since you were the one reviewing my do-while PRs in general (do you want me to create an issue and/or remove the bits about UNUSED?)

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM

@vstinner
Copy link
Member

vstinner commented Sep 8, 2024

cc @vstinner since you were the one reviewing my do-while PRs in general (do you want me to create an issue and/or remove the bits about UNUSED?)

Note: I prefer to not review draft PRs in general.

@picnixz picnixz marked this pull request as ready for review September 8, 2024 11:32
@picnixz
Copy link
Contributor Author

picnixz commented Sep 8, 2024

Note: I prefer to not review draft PRs in general.

Oh sorry for that. I'll keep in mind in the future!

@vstinner vstinner merged commit 05a401a into python:main Sep 9, 2024
40 checks passed
@vstinner
Copy link
Member

vstinner commented Sep 9, 2024

In general, I dislike PRs which only change coding style, but reusing the Py_UNUSED() macro is a good thing.

@picnixz picnixz deleted the chore/decimal-module-macro-cleanup branch September 9, 2024 09:37
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