-
Notifications
You must be signed in to change notification settings - Fork 792
Update J2CLItableMerging to consider custom descriptors #7729
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
base: main
Are you sure you want to change the base?
Conversation
continue; | ||
} | ||
vtabletype = descriptor; | ||
itabletype = type.fields[0].type.getHeapType(); |
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 might want to reserve field 0 in the descriptor. Once we have JS interop, the JS prototype will (almost certainly) have to be stored in field 0.
if (!typeNameInfo.fieldNames.count(1) || | ||
!typeNameInfo.fieldNames[1].equals("itable")) { |
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.
It seems we should have a similar check before looking up the name of field 0. Also, we can avoid double lookup by doing this:
if (!typeNameInfo.fieldNames.count(1) || | |
!typeNameInfo.fieldNames[1].equals("itable")) { | |
if (auto it = typeNameInfo.fieldNames.find(1); | |
it == typeNameInfo.fieldNames.end() || it->second.equals("itable")) { |
std::optional<HeapType> vtabletype; | ||
std::optional<HeapType> itabletype; |
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.
If these would be empty, then we'll just continue
to the next loop iteration anyway, so I don't see a reason to make them optional.
std::optional<HeapType> vtabletype; | |
std::optional<HeapType> itabletype; | |
HeapType vtabletype; | |
HeapType itabletype; |
.fields[0] | ||
.type)); | ||
|
||
HeapType& javaClass = parent.structInfoByITableType[curr->type.getHeapType()]->javaClass; |
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.
A HeapType is represented as a single number, so no need to take a reference to it unless you're going to mutate it.
HeapType& javaClass = parent.structInfoByITableType[curr->type.getHeapType()]->javaClass; | |
HeapType javaClass = parent.structInfoByITableType[curr->type.getHeapType()]->javaClass; |
Update J2CLItableMerging to consider types whose vtables are custom descriptors