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.

Sign up for GitHub

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

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
+438 −76
Open

Implementation of 'com_record' as a subclassable Python type. #2437

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
ebfaa62
Implementation of 'com_record' as a subclassable Python type.
geppi Sep 13, 2024
f3bc21a
Removed an unnecessary type cast and unreferenced local variables.
geppi Jan 3, 2025
010a6b3
Added a global dictionary for 'pythoncom.com_record' subclass registr…
geppi Jan 5, 2025
b4618a9
The algorithm that identifies the subclass of 'pythoncom.com_record'
geppi Jan 5, 2025
0f95786
Added the function 'register_record_class' to add proper subclasses of
geppi Jan 5, 2025
6b16620
Added a check for the record type name in the 'register_record_class'
geppi Jan 7, 2025
2b84fde
Update com/win32com/client/__init__.py
geppi Jan 7, 2025
ca4f490
Code cleanup.
geppi Jan 8, 2025
f92cc97
Reverted the part of commit cd1c2fcfc655f4981c86ece0e742927e0781dfc3
geppi Jan 9, 2025
b1e55d2
Added the dunder attribute '__record_type_guid__' to the
geppi Jan 9, 2025
c9aeec2
Added a NULL check for 'PyRecord::new_record' before calling
geppi Jan 9, 2025
cb675ab
Added tests for the 'pythoncom.com_record' subclassing functionality.
geppi Jan 9, 2025
61662ab
Added a NULL check for 'PyRecord::new_record' before calling
geppi Jan 12, 2025
aed5d91
Added a method for the 'tp_init' slot of 'com_record' to enable
geppi Jan 13, 2025
13bc5d7
Merge branch 'mhammond:main' into record_subclasses
geppi Jan 14, 2025
bd5efdc
Added a changelog entry.
geppi Jan 14, 2025
dfeec71
Added changes from code review.
geppi Jan 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
181 changes: 137 additions & 44 deletions com/win32com/src/PyRecord.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
#include <new>
#include "stdafx.h"
#include "PythonCOM.h"
#include "PyRecord.h"
Expand Down Expand Up @@ -31,7 +32,7 @@ class PyRecordBuffer {
long ref;
};

BOOL PyRecord_Check(PyObject *ob) { return ((ob)->ob_type == &PyRecord::Type); }
BOOL PyRecord_Check(PyObject *ob) { return PyObject_IsInstance(ob, (PyObject *)&PyRecord::Type); }

BOOL PyObject_AsVARIANTRecordInfo(PyObject *ob, VARIANT *pv)
{
Expand Down Expand Up @@ -91,7 +92,7 @@ PyObject *PyObject_FromSAFEARRAYRecordInfo(SAFEARRAY *psa)
hr = info->RecordCopy(source_data, this_dest_data);
if (FAILED(hr))
goto exit;
PyTuple_SET_ITEM(ret_tuple, i, new PyRecord(info, this_dest_data, owner));
PyTuple_SET_ITEM(ret_tuple, i, PyRecord::new_record(info, this_dest_data, owner));
Copy link
Owner

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.

Copy link
Contributor Author

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?

Copy link
Owner

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 of PyRecord::new_record will do the right thing if null/false is returned - but what the docs say will not be Ok is PyTuple_SET_ITEM.

Copy link
Contributor Author

@geppi geppi Jan 9, 2025 •
edited
Loading

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.

this_dest_data += cb_elem;
source_data += cb_elem;
}
Expand Down Expand Up @@ -141,7 +142,7 @@ PyObject *PyObject_FromRecordInfo(IRecordInfo *ri, void *data, ULONG cbData)
delete owner;
return PyCom_BuildPyException(hr, ri, IID_IRecordInfo);
}
return new PyRecord(ri, owner->data, owner);
return (PyObject *)PyRecord::new_record(ri, owner->data, owner);
geppi marked this conversation as resolved.
Show resolved Hide resolved
}

// @pymethod <o PyRecord>|pythoncom|GetRecordFromGuids|Creates a new record object from the given GUIDs
Expand Down Expand Up @@ -200,14 +201,65 @@ PyObject *pythoncom_GetRecordFromTypeInfo(PyObject *self, PyObject *args)
return ret;
}

PyRecord::PyRecord(IRecordInfo *ri, PVOID data, PyRecordBuffer *owner)
// This function creates a new 'com_record' instance with placement new.
// If the particular Record GUID belongs to a directly derived subclass
// of the 'com_record' base type, it instantiates this subclass.
Copy link
Owner

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?

Copy link
Contributor Author

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 the PyRecord::new_record function. Therefore I would prefer to keep this extra parameter which shortcuts the type identification procedure and just update the comment.

PyRecord *PyRecord::new_record(IRecordInfo *ri, PVOID data, PyRecordBuffer *owner)
{
PyObject *list, *raw, *ref;
Py_ssize_t i;
GUID structguid;
OLECHAR *guidString;
// By default we create an instance of the base 'com_record' type.
PyTypeObject *type = &PyRecord::Type;
// Retrieve the GUID of the Record to be created.
HRESULT hr = ri->GetGuid(&structguid);
if (FAILED(hr)) {
PyCom_BuildPyException(hr);
return NULL;
}
if (S_OK != StringFromCLSID(structguid, &guidString))
return NULL;
// Obtain a copy of the subclasses list to iterate over.
list = PyObject_CallMethod((PyObject *)type, "__subclasses__", NULL);
Copy link
Owner

Choose a reason for hiding this comment

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

not sure I like this as IIUC it makes the world non-deterministic, in that how this works will depends on what else happens to have been imported. Another alternative might be a function which allows you to register the subclass by GUID, and store them in a global map of some sort? I guess you could argue that's still non-deterministic, but at least requires explicit calls rather than side-effects)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please excuse my ignorance but I'm afraid I don't understand why this does depend on side effects.
Maybe you can point me on it with the help of the following examples.

Let's start with the plain vanilla pythoncom.com_record class:

import pythoncom

TL_CLSID = '{E6F07342-C1F7-4E4E-B021-11BBD54B9F37}'
TL_MJVER = 2
TL_MNVER = 3
TL_LCID = 0

T_ENTITY_CLSID = '{9F2C4E2E-2C5C-4F39-9FDB-840A1E08B165}'
T_LINE_CLSID = '{B1461DD4-8C86-4627-B444-3D833C980111}'

print(pythoncom.com_record.__subclasses__())

[]

and create an instance of the T_LINE COM Record:

line_1 = pythoncom.GetRecordFromGuids(TL_CLSID, TL_MJVER, TL_MNVER, TL_LCID, T_LINE_CLSID)
print(line_1)
print(type(line_1))

com_struct(startX=0.0, startY=0.0, startZ=0.0, endX=0.0, endY=0.0, endZ=0.0)
<class 'com_record'>

If we now define the subclasses of pythoncom.com_record with the appropriate GUIDs for the T_ENTITY and T_LINE COM Records from the TypeLibrary:

class T_ENTITY(pythoncom.com_record):
    __slots__ = tuple()
    TLBID = TL_CLSID
    MJVER = TL_MJVER
    MNVER = TL_MNVER
    LCID = TL_LCID
    GUID = T_ENTITY_CLSID

    
class T_LINE(pythoncom.com_record):
    __slots__ = tuple()
    TLBID = TL_CLSID
    MJVER = TL_MJVER
    MNVER = TL_MNVER
    LCID = TL_LCID
    GUID = T_LINE_CLSID

print(pythoncom.com_record.__subclasses__())

[<class 'main.T_ENTITY'>, <class 'main.T_LINE'>]

we can do:

line_2 = T_LINE()
print(line_2)
print(type(line_2))

com_struct(startX=0.0, startY=0.0, startZ=0.0, endX=0.0, endY=0.0, endZ=0.0)
<class 'main.T_LINE'>

The type of the COM Record instance line_1, which we had created before the subclasses were defined, of course didn't change:

print(type(line_1))

<class 'com_record'>

However, creating a new T_LINE COM Record instance with the factory function, does now also return an instance of the subclass.

line_3 = pythoncom.GetRecordFromGuids(TL_CLSID, TL_MJVER, TL_MNVER, TL_LCID, T_LINE_CLSID)
print(line_3)
print(type(line_3))

com_struct(startX=0.0, startY=0.0, startZ=0.0, endX=0.0, endY=0.0, endZ=0.0)
<class 'main.T_LINE'>

If we define an invalid COM Record subclass using a GUID that is not contained in the TypeLibrary:

T_BAD_CLSID = '{A50732E7-F546-498B-AF28-E58161F9CA5C}'

class T_BAD(pythoncom.com_record):
    __slots__ = tuple()
    TLBID = TL_CLSID
    MJVER = TL_MJVER
    MNVER = TL_MNVER
    LCID = TL_LCID
    GUID = T_BAD_CLSID

print(pythoncom.com_record.__subclasses__())

[<class 'main.T_ENTITY'>, <class 'main.T_LINE'>, <class 'main.T_BAD'>]

the attempt to create an instance fails with an exception:

bad = T_BAD()

---------------------------------------------------------------------------
com_error Traceback (most recent call last)
----> bad = T_BAD()
com_error: (-2147319765, 'Element nicht gefunden.', None, None)

Of course you are free to define all kinds of dysfunctional subclasses:

class T_STUPID(pythoncom.com_record):
    description = 'blabla'

print(pythoncom.com_record.__subclasses__())

[<class 'main.T_ENTITY'>, <class 'main.T_LINE'>, <class 'main.T_BAD'>, <class 'main.T_STUPID'>]

but you cannot instantiate them:

gaga = T_STUPID()

---------------------------------------------------------------------------
SystemError Traceback (most recent call last)
----> gaga = T_STUPID()
SystemError: <class 'main.T_STUPID'> returned NULL without setting an exception

So to define working subclasses of pythoncom.com_record you have to follow the recipe and provide the GUID of the TypeLibrary that containts the COM Record definition, as well as its major and minor version number, plus the LCID and last but not least the GUID of the COM Record type as class variables.

I would argue that the definition of such a subclass is a kind of global registration.

Please excuse if I completely missed the point with the above examples but hopefully they can help to identify where there are side effects or non-determinisms.

Copy link
Owner

@mhammond mhammond Jan 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

My point is that these subclasses are presumably going to be in your code rather than in pywin32. Thus:

print(pythoncom.com_record.__subclasses__())

[<class 'main.T_ENTITY'>, <class 'main.T_LINE'>]

Assuming T_ENTITY/T_LINE etc are in modules you wrote, then the print above will only generate that output if those modules you wrote were actually imported. Thus, this new capability will implicitly depend on exactly what modules were previously imported and presumably what order they are in when multiple modules try and register the same type.

I would argue that the definition of such a subclass is a kind of global registration.

That's true, but it's implicit and hard to reason about. An explicit registration makes more sense to be because (a) it's easier to locate where these calls are and (b) there's an opportunity to fail if 2 callers try and register the same type. I'm saying that without (b), which type is used is largely non-deterministic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, understood.
Do you think it is necessary to implement the nuts and bolts of this registration in pythoncom or would it be sufficient to keep the implementation as is and just provide an 'official' class-factory function for pythoncom.com_record subclasses that does the registration and checks to prevent double registration?

I mean the whole class machinery of Python itself does not prevent from doing all kinds of weird things and in particular somehow related to your point (b):

class A():
    a = 1

class B(A):
    b = 2

class B(A):
    c = 3

print(A.__subclasses__())

[<class 'main.B'>, <class 'main.B'>]

instantiating B does return an instance of the class that was last defined with this name:

x = B()

print(x.b)

---------------------------------------------------------------------------
AttributeError Traceback (most recent call last)
x = B()
----> print(x.b)
AttributeError: 'B' object has no attribute 'b'

print(x.c)

3

The class registration function could easily be written in Python and use the machinery as implemented.
Of course users could then still exploit the machinery under the hood and invoke the implicit behavior.
However, if you're not using the 'official' method you should know what you do.

Copy link
Owner

Choose a reason for hiding this comment

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

A Python implementation seems fine to me and I'm not bothered that people can arrange to avoid this - it's just the fact that a base class which mediates which subclass is instantiated just smells inverted to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's just the fact that a base class which mediates which subclass is instantiated just smells inverted to me.

The issue for which I couldn't come up with a better solution is how to instantiate the proper pythoncom.com_record subclass for Records returned by a COM method call. Without the 'smelly' part everything was fine as long as the Records were created by client code. However, all Records returned from a COM method call were just generic base class pythoncom.com_record instances. Therefore I introduced this 'identification' logic based on the GUID of the returned Record.

Copy link
Owner

Choose a reason for hiding this comment

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

I was thinking get_registered_record_class or similar would hold classes, so the logic around here would be roughly the same - you are still making a call back into Python, but instead of subclasses you would be call the new function with some args. You would then expect the result to be a single class (or None) rather than iterating here - which you'd instantiate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added the dictionary 'RecordClasses' to pythoncom.

The function 'PyRecord::new_record' does now check if the COM Record GUID is contained in this dict and uses the value returned for the GUID key as the subclass to instantiate.

The new Python function 'register_record_class' in the client module can be used to populate the dictionary with 'GUID' key / 'com_record subclass' value pairs. It checks that the value to be registered is a proper, instantiable com_record subclass and that the GUID is not already contained in the dictionary thus preventing accidential, potentially conflicting double registration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While working on the tests, I recognized that it might be confusing that you could register a subclass with a name different from the name for that GUID in the TypeLibrary. In general it shouldn't matter what name you give to a struct because it's identity is based on the GUID. However, it might be confusing if you have to use a different name in 'win32com.client.Record' to create the struct and the returned instance is then of a type with the registered name.

There are pros and cons. On the one hand it could be helpful to give COM Record types a more meaningful name with respect to your application than the one from the TypeLibrary. On the other hand it might create inconsistencies when mixing subclass instance creation with the use of 'win32com.client.Record'.
Although one could argue that a user who defines and registers a COM Record type with a different name should be aware of the consequences.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've given this a second thought and would value consistency over flexibility.

Therefore I've added a check for the record type name in the 'register_record_class' Python function.
To enable this in a straight forward way I've also added another dunder attribute '__record_type_name__' to the 'PyRecord::getattro' function that retrieves the record type name via 'IRecordInfo::GetName'.

If someone would really like to give the subclass a different name from the name in the TypeLibrary there is still the possibility to bypass the registration function and enter the class directly into the 'pythoncom.RecordClasses' dictionary.
However, using the "official" way everything should be consistent now.

// We now have a list of the directly derived subclasses of 'com_record'.
// If no subclasses have been defined the list is empty.
// Iterate over the list and try to find a subclass with matching GUID.
PyObject *recordIter = PyObject_GetIter(list);
PyTypeObject *recordType;
wchar_t *item_guid;
while (recordType = (PyTypeObject *)PyIter_Next(recordIter)) {
if (PyObject *item = PyDict_GetItemString(recordType->tp_dict, "GUID")) {
if (!(item_guid = PyUnicode_AsWideCharString(item, NULL)))
continue;
if (wcscmp(guidString, item_guid) == 0) {
type = recordType;
PyMem_Free(item_guid);
break;
}
PyMem_Free(item_guid);
}
Py_DECREF(recordType);
}
Py_DECREF(recordIter);
Py_DECREF(list);
::CoTaskMemFree(guidString);
// Finally allocate the memory for the the appropriate
// Record type and construct the instance with placement new.
char *buf = (char *)PyRecord::Type.tp_alloc(type, 0);
if (buf == NULL) {
delete owner;
return NULL;
}
return new (buf) PyRecord(ri, owner->data, owner);
}

PyRecord::PyRecord(IRecordInfo *ri, PVOID data, PyRecordBuffer *buf_owner)
{
ob_type = &PyRecord::Type;
_Py_NewReference(this);
ri->AddRef();
pri = ri;
pdata = data;
this->owner = owner;
owner = buf_owner;
owner->AddRef();
};

Expand All @@ -217,44 +269,81 @@ PyRecord::~PyRecord()
pri->Release();
}

PyObject *PyRecord::tp_new(PyTypeObject *type, PyObject *args, PyObject *kwds)
{
PyObject *item, *obGuid, *obInfoGuid;
int major, minor, lcid;
GUID guid, infoGuid;
if (type == &PyRecord::Type)
// If the base 'com_record' type was called try to get the
// information required for instance creation from the call parameters.
{
if (!PyArg_ParseTuple(args, "OiiiO:__new__",
&obGuid, // @pyparm <o PyIID>|iid||The GUID of the type library
&major, // @pyparm int|verMajor||The major version number of the type lib.
&minor, // @pyparm int|verMinor||The minor version number of the type lib.
&lcid, // @pyparm int|lcid||The LCID of the type lib.
&obInfoGuid)) // @pyparm <o PyIID>|infoIID||The GUID of the record info in the library
return NULL;
if (!PyWinObject_AsIID(obGuid, &guid))
return NULL;
if (!PyWinObject_AsIID(obInfoGuid, &infoGuid))
return NULL;
}
// Otherwise try to get the information from the class variables of the derived type.
else if (!(item = PyDict_GetItemString(type->tp_dict, "GUID")) || !PyWinObject_AsIID(item, &infoGuid) ||
!(item = PyDict_GetItemString(type->tp_dict, "TLBID")) || !PyWinObject_AsIID(item, &guid) ||
!(item = PyDict_GetItemString(type->tp_dict, "MJVER")) || ((major = PyLong_AsLong(item)) == -1) ||
!(item = PyDict_GetItemString(type->tp_dict, "MNVER")) || ((minor = PyLong_AsLong(item)) == -1) ||
!(item = PyDict_GetItemString(type->tp_dict, "LCID")) || ((lcid = PyLong_AsLong(item)) == -1))
return NULL;
IRecordInfo *ri = NULL;
HRESULT hr = GetRecordInfoFromGuids(guid, major, minor, lcid, infoGuid, &ri);
if (FAILED(hr))
return PyCom_BuildPyException(hr);
PyObject *ret = PyObject_FromRecordInfo(ri, NULL, 0);
ri->Release();
return ret;
}

PyTypeObject PyRecord::Type = {
PYWIN_OBJECT_HEAD "com_record",
sizeof(PyRecord),
0,
PyRecord::tp_dealloc, /* tp_dealloc */
0, /* tp_print */
0, /* tp_getattr */
0, /* tp_setattr */
0, /* tp_compare */
&PyRecord::tp_repr, /* tp_repr */
0, /* tp_as_number */
0, /* tp_as_sequence */
0, /* tp_as_mapping */
0, /* tp_hash */
0, /* tp_call */
0, /* tp_str */
PyRecord::getattro, /* tp_getattro */
PyRecord::setattro, /* tp_setattro */
0, /* tp_as_buffer */
Py_TPFLAGS_DEFAULT, /* tp_flags */
0, /* tp_doc */
0, /* tp_traverse */
0, /* tp_clear */
PyRecord::tp_richcompare, /* tp_richcompare */
0, /* tp_weaklistoffset */
0, /* tp_iter */
0, /* tp_iternext */
PyRecord::methods, /* tp_methods */
0, /* tp_members */
0, /* tp_getset */
0, /* tp_base */
0, /* tp_dict */
0, /* tp_descr_get */
0, /* tp_descr_set */
0, /* tp_dictoffset */
0, /* tp_init */
0, /* tp_alloc */
0, /* tp_new */
(destructor)PyRecord::tp_dealloc, /* tp_dealloc */
0, /* tp_print */
0, /* tp_getattr */
0, /* tp_setattr */
0, /* tp_compare */
&PyRecord::tp_repr, /* tp_repr */
0, /* tp_as_number */
0, /* tp_as_sequence */
0, /* tp_as_mapping */
0, /* tp_hash */
0, /* tp_call */
0, /* tp_str */
PyRecord::getattro, /* tp_getattro */
PyRecord::setattro, /* tp_setattro */
0, /* tp_as_buffer */
Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE, /* tp_flags */
0, /* tp_doc */
0, /* tp_traverse */
0, /* tp_clear */
PyRecord::tp_richcompare, /* tp_richcompare */
0, /* tp_weaklistoffset */
0, /* tp_iter */
0, /* tp_iternext */
PyRecord::methods, /* tp_methods */
0, /* tp_members */
0, /* tp_getset */
0, /* tp_base */
0, /* tp_dict */
0, /* tp_descr_get */
0, /* tp_descr_set */
0, /* tp_dictoffset */
0, /* tp_init */
0, /* tp_alloc */
(newfunc)PyRecord::tp_new, /* tp_new */
};

static PyObject *PyRecord_reduce(PyObject *self, PyObject *args)
Expand Down Expand Up @@ -496,7 +585,7 @@ PyObject *PyRecord::getattro(PyObject *self, PyObject *obname)
// Short-circuit sub-structs and arrays here, so we don't allocate a new chunk
// of memory and copy it - we need sub-structs to persist.
if (V_VT(&vret) == (VT_BYREF | VT_RECORD))
return new PyRecord(V_RECORDINFO(&vret), V_RECORD(&vret), pyrec->owner);
return PyRecord::new_record(V_RECORDINFO(&vret), V_RECORD(&vret), pyrec->owner);
else if (V_VT(&vret) == (VT_BYREF | VT_ARRAY | VT_RECORD)) {
SAFEARRAY *psa = *V_ARRAYREF(&vret);
if (SafeArrayGetDim(psa) != 1)
Expand Down Expand Up @@ -531,7 +620,7 @@ PyObject *PyRecord::getattro(PyObject *self, PyObject *obname)
// in the last parameter, i.e. 'sub_data == NULL'.
this_data = (BYTE *)psa->pvData;
for (i = 0; i < nelems; i++) {
PyTuple_SET_ITEM(ret_tuple, i, new PyRecord(sub, this_data, pyrec->owner));
PyTuple_SET_ITEM(ret_tuple, i, PyRecord::new_record(sub, this_data, pyrec->owner));
this_data += element_size;
}
array_end:
Expand Down Expand Up @@ -645,4 +734,8 @@ PyObject *PyRecord::tp_richcompare(PyObject *self, PyObject *other, int op)
return ret;
}

void PyRecord::tp_dealloc(PyObject *ob) { delete (PyRecord *)ob; }
void PyRecord::tp_dealloc(PyRecord *self)
{
self->~PyRecord();
Py_TYPE(self)->tp_free((PyObject *)self);
}
4 changes: 4 additions & 0 deletions com/win32com/src/PythonCOM.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2248,6 +2248,10 @@ PYWIN_MODULE_INIT_FUNC(pythoncom)
PyType_Ready(&PyRecord::Type) == -1)
PYWIN_MODULE_INIT_RETURN_ERROR;

// Add the PyRecord type as a module attribute
if (PyModule_AddObject(module, "com_record", (PyObject *)&PyRecord::Type) != 0)
Copy link
Owner

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!)

Copy link
Contributor Author

@geppi geppi Jan 18, 2025 •
edited
Loading

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

pythoncom.com_record
<class 'com_record'>

and the type of an instance is

type(instance)
<class 'com_record'>

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:

pythoncom.com_record_type
<class 'com_record'>

Up to you.

PYWIN_MODULE_INIT_RETURN_ERROR;

// Setup our sub-modules
if (!initunivgw(dict))
PYWIN_MODULE_INIT_RETURN_ERROR;
Expand Down
4 changes: 3 additions & 1 deletion com/win32com/src/include/PyRecord.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,9 @@ class PyRecord : public PyObject {
PyRecord(IRecordInfo *ri, PVOID data, PyRecordBuffer *owner);
~PyRecord();

static void tp_dealloc(PyObject *ob);
static PyRecord *new_record(IRecordInfo *ri, PVOID data, PyRecordBuffer *owner);
static PyObject *tp_new(PyTypeObject *type, PyObject *args, PyObject *kwds);
static void tp_dealloc(PyRecord *ob);
static PyObject *getattro(PyObject *self, PyObject *obname);
static int setattro(PyObject *self, PyObject *obname, PyObject *v);
static PyObject *tp_repr(PyObject *self);
Expand Down