Skip to content

wip: structural map#649

Open
Kathryn-cat wants to merge 1 commit into
apache:mainfrom
Kathryn-cat:kathrync/map
Open

wip: structural map#649
Kathryn-cat wants to merge 1 commit into
apache:mainfrom
Kathryn-cat:kathrync/map

Conversation

@Kathryn-cat

Copy link
Copy Markdown
Contributor

No description provided.

@Kathryn-cat Kathryn-cat marked this pull request as ready for review June 29, 2026 05:58

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request introduces the StructuralMapper and StructuralMapperObj APIs in include/tvm/ffi/extra/structural_map.h to support structural mapping and in-place mutation of object-backed values. It also adds corresponding custom hooks (kStructuralMap and kStructuralInplaceMutate), registers test leaf objects, and includes comprehensive unit tests. The review feedback suggests two minor optimizations in structural_map.h: avoiding a const_cast when obtaining the field address, and reusing the calculated field address instead of recalculating it for the field setter.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +508 to +510
const void* field_addr = reinterpret_cast<const char*>(new_obj) + field_info->offset;
int ret_code = field_info->getter(const_cast<void*>(field_addr),
reinterpret_cast<TVMFFIAny*>(&field_value));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Since new_obj is a non-const pointer (Object*), we can avoid the const_cast on field_addr by declaring it as void* and using reinterpret_cast<char*>(new_obj).

        void* field_addr = reinterpret_cast<char*>(new_obj) + field_info->offset;
        int ret_code = field_info->getter(field_addr,
                                          reinterpret_cast<TVMFFIAny*>(&field_value));

Comment on lines +538 to +540
void* new_field_addr = reinterpret_cast<char*>(new_obj) + field_info->offset;
ret_code = reflection::CallFieldSetter(field_info, new_field_addr,
reinterpret_cast<const TVMFFIAny*>(&new_field));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

We can reuse the field_addr variable calculated earlier at line 508 instead of redundantly recalculating it as new_field_addr here.

        ret_code = reflection::CallFieldSetter(field_info, field_addr,
                                               reinterpret_cast<const TVMFFIAny*>(&new_field));

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant