[FEAT] Introduce ABI-friendly structural visitor and walk traversal API#601
[FEAT] Introduce ABI-friendly structural visitor and walk traversal API#601Kathryn-cat wants to merge 1 commit into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a structural visit and walk implementation for the TVM FFI, enabling traversal of object graphs with support for early interruption. It adds the StructuralVisitor and StructuralWalk APIs, implements built-in container visitors, and provides unsafe raw-storage helpers for Expected. The review feedback recommends adding a defensive check for undefined ObjectRef values in DefaultVisitExpected to prevent potential crashes, supporting void return types in StructuralWalk callbacks to reduce boilerplate, and adding a static assertion to verify that each callback takes exactly one argument to avoid obscure template compilation errors.
Kathryn-cat
force-pushed
the
kathryncat/visitor
branch
from
6618b29 to
802b33e
Compare
Kathryn-cat
force-pushed
the
kathryncat/visitor
branch
6 times, most recently
from
d1858f6 to
e7da414
Compare
| // --------------------------------------------------------------------------- | ||
|
|
||
| class TLeafObj : public Object { | ||
| public: |
There was a problem hiding this comment.
consider consolidate sme of the objects to testing_object.h
| std::vector<ObjectRef> visited; | ||
| std::vector<TVMFFIDefRegionKind> modes; | ||
| ObjectRef interrupt_on; | ||
| String interrupt_payload = "stop"; |
There was a problem hiding this comment.
This is likely not need atm, focus ontesting StructuralWalk
| class TIgnoredObj : public Object { | ||
| public: | ||
| ObjectRef visible; | ||
| ObjectRef ignored; |
There was a problem hiding this comment.
this can be skiped for now, to test ignored, have something lke binding with a comment field that is ignored, no need to have dedicated class test
| ObjectRef non_recursive; | ||
| ObjectRef plain_after_non_recursive; | ||
|
|
||
| TDefRegionObj(ObjectRef recursive, ObjectRef plain_after_recursive, ObjectRef non_recursive, |
There was a problem hiding this comment.
this is not needed since the node is quite artifical, good to have some reasonably common nodes, TPair, with function def(params, body) plus comment likely suffice to cover cases of interest, cross check testing_object.h
| } | ||
|
|
||
| TEST(StructuralVisitor, DefaultVisitTraversesList) { | ||
| ObjectRef lhs = TLeaf("lhs"); |
There was a problem hiding this comment.
always use StructuralWalk to test, test a few cases of composed containers, no need to have one for each
| } | ||
| // Decode by moving from owned Any storage after a strict type check. | ||
| TVM_FFI_INLINE static WalkResult MoveFromAnyAfterCheck(TVMFFIAny* src) { | ||
| return WalkResult(Base::MoveFromAnyAfterCheck(src)); |
There was a problem hiding this comment.
// Auto-convert None -> Advance
if (src->type_index == TypeIndex::kTVMFFINone) {
return WalkResult::Advance();
}
implement auto convert None to Advance, so ffi function can return None (or no return which translate to none) and implies advance
do it for the few places that involves conversion
Kathryn-cat
force-pushed
the
kathryncat/visitor
branch
from
e7da414 to
014afc9
Compare
Kathryn-cat
force-pushed
the
kathryncat/visitor
branch
from
1448f2a to
9838ea8
Compare
2 participants
No description provided.