Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Allow more methods to take interned arguments #2312
Allow more methods to take interned arguments #2312
Changes from 6 commits
8e2e3af
7f96584
fe49a81
ceaf835
c7319e1
90db8a2
b75c3e9
f1a926f
cf07605
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Sorry this is a bit messy since resolving the merge conflict.
How do you feel about:
versus
(or something else?)
Personally I prefer the latter. IMO the former is error prone and gives a bad example.
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 feel the former is error prone? I think the second is more error prone because missing the
Py_DECREF
call is leaking memory just like missingfree()
in C.Perhaps a middle ground is to make it obvious we're using RAII through
Py
?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'm mostly worried about someone reading it and and writing it slightly differently:
which produces a dangling pointer, as the python object is dropped at the end of the statement (I should add documentation about this).
Sure 👍
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 is an excellent point which I'd totally missed. IMO the middle ground above is probably the only sensible way to arrange this then. If you like, I'll submit a PR to fix the whole codebase to use this pattern once this PR merged? That way you can just worry about the modified functions here without another merge conflict.