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
Implementation of 'com_record' as a subclassable Python type. #2437
base: main
Are you sure you want to change the base?
Implementation of 'com_record' as a subclassable Python type. #2437
Changes from 6 commits
ebfaa62
f3bc21a
010a6b3
b4618a9
0f95786
6b16620
2b84fde
ca4f490
f92cc97
b1e55d2
c9aeec2
cb675ab
61662ab
aed5d91
13bc5d7
bd5efdc
dfeec71
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.
We should probably add a null check? It should have already been there for
new
, but there are many more error conditions now.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.
You're right. The question is where to put this null check?
There are other places where PyRecord::new_record is used without a null check.
Should we instead raise a Python exception in PyRecord::new_record itself in every place where it currently returns a null without raising an exception?
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.
IIUC, it will only currently return null when OOM. In this patch we have a more complicated situation - sometimes when it returns null there will be an exception set, whereas OOM will not. Having those OOM cases call
PyErr_NoMem()
seems easy, and it looks to me like all of the callers ofPyRecord::new_record
will do the right thing if null/false is returned - but what the docs say will not be Ok isPyTuple_SET_ITEM
.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've added commit c9aeec2 and commit 61662ab to resolve this.
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 comment needs an update for the param. Or maybe a new function
PyRecord::new_record_with_type
taking the extra param, which helps move the type-finding out?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.
The tp_new method is the only code path that would use such a
PyRecord::new_record_with_type
function and it would duplicate code that's already in thePyRecord::new_record
function. Therefore I would prefer to keep this extra parameter which shortcuts the type identification procedure and just update the comment.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.
should it have "type" in the name? (I've no idea about idiomatic python tbh!)
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.
Well, in the example code on python.org it has the same name as in the tp_name slot. So the module attribute is
and the type of an instance is
which, I would say, makes sense.
Although more precisely, in the example code, the tp_name slot also specifies the module name of the new type.
However, I didn't want to change this to
PYWIN_OBJECT_HEAD "pythoncom.com_record"
because I didn't want to break any existing code.Of course the name of the module attribute could be changed to
com_record_type
which would only make a difference in one place as far as I understand:Up to you.