Fix: AttributeError with anonymous unions #57
Conversation
|
This looks good to me. @aaronj0 can you take a look? What’s the reason for the failing bot, is it still missing some suppression configuration? |
Yep, I think we are using the latest llvm19(rc2) on these repos, but not on InterOp(rc1), hence the discrepancy. |
|
@aaronj0, should we merge this or we should wait for you to update the suppression file? |
We don't need to add the suppression now, but I have left some review comments that would be great if addressed |
I don’t see any review comments. Can you click on finish review? |
|
@aaronj0 ping. |
| // 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) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| 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)); | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
|
@aaronj0 @vgvassilev |
That is great. Can you open a PR against cppyy to enable the tests that this fixed? |
Fixes: https://github.com/Vipul-Cariappa/cppyy-compiler-research/blob/aafa1837b21a5e5f9a1282ecf5f3fef688761e96/test/test_datatypes.py#L1562-L1653 test case.