Skip to content

Fix: AttributeError with anonymous unions #57

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

Closed
wants to merge 1 commit into from
Closed

Fix: AttributeError with anonymous unions #57

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 4 additions & 2 deletions src/CPPDataMember.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,11 +307,13 @@ PyTypeObject CPPDataMember_Type = {
} // namespace CPyCppyy

//- public members -----------------------------------------------------------
void CPyCppyy::CPPDataMember::Set(Cppyy::TCppScope_t scope, Cppyy::TCppScope_t data)
void CPyCppyy::CPPDataMember::Set(Cppyy::TCppScope_t scope, Cppyy::TCppScope_t data,
intptr_t additional_offset)
{
fEnclosingScope = scope;
fScope = data;
fOffset = Cppyy::GetDatamemberOffset(data); // XXX: Check back here // TODO: make lazy
fOffset = Cppyy::GetDatamemberOffset(data) +
additional_offset; // XXX: Check back here // TODO: make lazy
fFlags = Cppyy::IsStaticDatamember(data) ? kIsStaticData : 0;

const std::string name = Cppyy::GetFinalName(data);
Expand Down
7 changes: 4 additions & 3 deletions src/CPPDataMember.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@ class CPPInstance;

class CPPDataMember {
public:
void Set(Cppyy::TCppScope_t scope, Cppyy::TCppScope_t var);
void Set(Cppyy::TCppScope_t scope, Cppyy::TCppScope_t var,
intptr_t additional_offset=0);
void Set(Cppyy::TCppScope_t scope, const std::string& name, void* address);

std::string GetName();
Expand Down Expand Up @@ -56,12 +57,12 @@ inline bool CPPDataMember_CheckExact(T* object)

//- creation -----------------------------------------------------------------
inline CPPDataMember* CPPDataMember_New(
Cppyy::TCppScope_t scope, Cppyy::TCppScope_t var)
Cppyy::TCppScope_t scope, Cppyy::TCppScope_t var, intptr_t additional_offset=0)
{
// Create an initialize a new property descriptor, given the C++ datum.
CPPDataMember* pyprop =
(CPPDataMember*)CPPDataMember_Type.tp_new(&CPPDataMember_Type, nullptr, nullptr);
pyprop->Set(scope, var);
pyprop->Set(scope, var, additional_offset);
return pyprop;
}

Expand Down
70 changes: 36 additions & 34 deletions src/ProxyWrappers.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,10 +89,42 @@ static PyObject* CreateNewCppProxyClass(Cppyy::TCppScope_t klass, PyObject* pyba
}

static inline
void AddPropertyToClass(PyObject* pyclass,
Cppyy::TCppScope_t scope, Cppyy::TCppScope_t data)
void AddPropertyToClass(PyObject* pyclass, Cppyy::TCppScope_t scope,
Cppyy::TCppScope_t data, intptr_t additional_offset=0)
{
CPyCppyy::CPPDataMember* property = CPyCppyy::CPPDataMember_New(scope, data);
if (!Cppyy::IsPublicData(data))
return;

// enum datamembers (this in conjunction with previously collected enums above)
if (Cppyy::IsEnumType(Cppyy::GetDatamemberType(data)) &&
Cppyy::IsStaticDatamember(data)) {
// some implementation-specific data members have no address: ignore them
if (!Cppyy::GetDatamemberOffset(data))
return;

// two options: this is a static variable, or it is the enum value, the latter
// already exists, so check for it and move on if set
PyObject* eset = PyObject_GetAttrString(pyclass,
const_cast<char*>(Cppyy::GetFinalName(data).c_str()));
if (eset) {
Py_DECREF(eset);
return;
}

PyErr_Clear();
}

if (strstr(Cppyy::GetDatamemberTypeAsString(data).c_str(), "anonymous struct at") ||
strstr(Cppyy::GetDatamemberTypeAsString(data).c_str(), "anonymous union at")) {
std::vector<Cppyy::TCppScope_t> datamembers = Cppyy::GetDatamembers(Cppyy::GetTypeScope(data));
for (auto &datamember: datamembers) {
// properties (aka public (static) data members)
AddPropertyToClass(pyclass, scope, datamember,
additional_offset + Cppyy::GetDatamemberOffset(data));
}
}
Comment on lines +117 to +125
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Recursion is used here.

I can undo the changes in BuildScopeProxyDict, but then I will have to duplicate the same things in AddPropertyToClass for recursion to work.


CPyCppyy::CPPDataMember* property = CPyCppyy::CPPDataMember_New(scope, data, additional_offset);
PyObject* pname = CPyCppyy_PyText_InternFromString(const_cast<char*>(property->GetName().c_str()));

// allow access at the instance level
Expand Down Expand Up @@ -371,37 +403,7 @@ static int BuildScopeProxyDict(Cppyy::TCppScope_t scope, PyObject* pyclass, cons
// collect data members (including enums)
std::vector<Cppyy::TCppScope_t> datamembers = Cppyy::GetDatamembers(scope);
for (auto &datamember : datamembers) {
// allow only public members
if (!Cppyy::IsPublicData(datamember))
continue;

// enum datamembers (this in conjunction with previously collected enums above)
if (Cppyy::IsEnumType(Cppyy::GetDatamemberType(datamember)) && Cppyy::IsStaticDatamember(datamember)) {
// some implementation-specific data members have no address: ignore them
if (!Cppyy::GetDatamemberOffset(datamember))
continue;

// two options: this is a static variable, or it is the enum value, the latter
// already exists, so check for it and move on if set
PyObject* eset = PyObject_GetAttrString(pyclass,
const_cast<char*>(Cppyy::GetFinalName(datamember).c_str()));
if (eset) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should get by with less refactoring (i.e. retain the changes in the anonymus enum block in BuildScopeProxyDict(adding the offset), instead of moving the whole block that collects datamembers to AddPropertyToClass

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The reason behind moving the entire logic into AddPropertyToClass is to be able to use recursion for nested anonymous structs and unions.
Test cases do not fail, so the refactor is safe.

Let me know if there is a specific change that looks off to you.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes I was wondering whether the recursion is a good idea but I guess since it only happens when we have anonymous structs and unions, it should not be too expensive in the normal case.

I was wondering how masters support nested anonymous structs and unions without recursion?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was wondering how masters support nested anonymous structs and unions without recursion?

I did not have a look at it. I saw the problem and came up with a solution. The solution looked simple and easy to implement. So I did not look at how master does this.

Copy link
Collaborator

@aaronj0 aaronj0 Sep 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I understand that but one of the main points here is not to diverge from master as much as possible. We can make as many changes in clingwrapper and add interfaces in InterOp to facilitate/emulate what is made available to CPyCppyy to achieve the same result. Since the support does not use recursion in master and is achieved with the interfaces at hand, I would recommend a side-by-side debug to emulate the same on the backend side. @vgvassilev what do you think?

Copy link

Choose a reason for hiding this comment

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

Yes, eventually we should merge all of this work back to upstream and if that change is not mergeable will be a problem. I cannot judge that now but perhaps we could discuss that at the meeting later today.

Py_DECREF(eset);
continue;
}

PyErr_Clear();

// it could still be that this is an anonymous enum, which is not in the list
// provided by the class
if (strstr(Cppyy::GetDatamemberTypeAsString(datamember).c_str(), "(anonymous)") != 0 ||
strstr(Cppyy::GetDatamemberTypeAsString(datamember).c_str(), "(unnamed)") != 0) {
AddPropertyToClass(pyclass, scope, datamember);
continue;
}
}

// properties (aka public (static) data members)
// properties (aka public (static) data members)
AddPropertyToClass(pyclass, scope, datamember);
}

Expand Down