Skip to content

Conversation

@junrushao
Copy link
Member

When generation function schema for member functions of a certain class, the existing logics overlooked this as the first argument, which, as a result, generates inproper number of arguments.

More specifically, in the snippet below,

class MyCls {
  void MyFunc(int a);
};

refl::GlobalDef()
    .def_method("MyCls_MyFunc", &MyCls::MyFunc);

the global method MyCls_MyFunc is supposed to have signature (MyCls, int) -> None, but the implementation on mainline gives (int) -> None.

This PR fixes this issue.

@junrushao junrushao marked this pull request as ready for review October 7, 2025 05:26
@junrushao
Copy link
Member Author

/gemini review

Copy link
Contributor

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

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 correctly addresses an issue where the self argument was overlooked in the schema generation for C++ member functions. The core fix in include/tvm/ffi/function_details.h correctly injects the this pointer type into the argument list. The accompanying test updates in both C++ and Python are thorough and validate the fix. Additionally, the PR includes some unrelated but beneficial changes, such as relaxing type argument constraints in type_info.pxi and adding more test coverage. Overall, this is a solid contribution that improves the correctness and robustness of the FFI reflection system. I have one minor suggestion for a follow-up improvement.

@junrushao junrushao requested a review from tqchen October 7, 2025 08:01
@tqchen tqchen merged commit 368af82 into apache:main Oct 7, 2025
7 checks passed
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.

2 participants