gemini-code-assist[bot] commented on code in PR #598:
URL: https://github.com/apache/tvm-ffi/pull/598#discussion_r3314252957
##########
src/ffi/extra/reflection_extra.cc:
##########
@@ -127,6 +129,61 @@ inline void AccessPathRegisterReflection() {
[](const AccessPath& self, const AccessPath& other) { return
self->PathEqual(other); });
}
+inline void AccessStepRegisterRepr() {
+ // Register __ffi_repr__ for AccessStep: format one step fragment.
+ // kAttr -> ".name"
+ // kArrayItem -> "[index]"
+ // kMapItem -> "[<repr(key)>]" (string keys are quoted via
fn_repr)
+ // kAttrMissing -> "[<missing:"name">]"
+ // kArrayItemMissing -> "[<missing:index>]"
+ // kMapItemMissing -> "[<missing:<repr(key)>>]"
+ namespace refl = tvm::ffi::reflection;
+ refl::TypeAttrDef<AccessStepObj>().def(
+ refl::type_attr::kRepr, [](AccessStep step, ffi::Function fn_repr) ->
ffi::String {
+ std::ostringstream os;
+ switch (step->kind) {
+ case AccessKind::kAttr:
+ os << "." << step->key.cast<ffi::String>();
+ break;
+ case AccessKind::kArrayItem:
+ os << "[" << step->key.cast<int64_t>() << "]";
+ break;
+ case AccessKind::kMapItem:
+ os << "[" << fn_repr(step->key).cast<ffi::String>() << "]";
+ break;
+ case AccessKind::kAttrMissing:
+ os << "[<missing:" << fn_repr(step->key).cast<ffi::String>() <<
">]";
+ break;
+ case AccessKind::kArrayItemMissing:
+ os << "[<missing:" << step->key.cast<int64_t>() << ">]";
+ break;
+ case AccessKind::kMapItemMissing:
+ os << "[<missing:" << fn_repr(step->key).cast<ffi::String>() <<
">]";
+ break;
+ }
Review Comment:
It is a good practice to include a `default` case in the `switch` statement
to handle any unexpected or invalid enum values defensively, especially since
these objects can be constructed or manipulated via FFI boundaries where type
safety might not be strictly enforced.
```c
switch (step->kind) {
case AccessKind::kAttr:
os << "." << step->key.cast<ffi::String>();
break;
case AccessKind::kArrayItem:
os << "[" << step->key.cast<int64_t>() << "]";
break;
case AccessKind::kMapItem:
os << "[" << fn_repr(step->key).cast<ffi::String>() << "]";
break;
case AccessKind::kAttrMissing:
os << "[<missing:" << fn_repr(step->key).cast<ffi::String>() <<
">]";
break;
case AccessKind::kArrayItemMissing:
os << "[<missing:" << step->key.cast<int64_t>() << ">]";
break;
case AccessKind::kMapItemMissing:
os << "[<missing:" << fn_repr(step->key).cast<ffi::String>() <<
">]";
break;
default:
TVM_FFI_UNREACHABLE();
}
```
##########
src/ffi/extra/reflection_extra.cc:
##########
@@ -127,6 +129,61 @@ inline void AccessPathRegisterReflection() {
[](const AccessPath& self, const AccessPath& other) { return
self->PathEqual(other); });
}
+inline void AccessStepRegisterRepr() {
+ // Register __ffi_repr__ for AccessStep: format one step fragment.
+ // kAttr -> ".name"
+ // kArrayItem -> "[index]"
+ // kMapItem -> "[<repr(key)>]" (string keys are quoted via
fn_repr)
+ // kAttrMissing -> "[<missing:"name">]"
+ // kArrayItemMissing -> "[<missing:index>]"
+ // kMapItemMissing -> "[<missing:<repr(key)>>]"
+ namespace refl = tvm::ffi::reflection;
+ refl::TypeAttrDef<AccessStepObj>().def(
+ refl::type_attr::kRepr, [](AccessStep step, ffi::Function fn_repr) ->
ffi::String {
+ std::ostringstream os;
+ switch (step->kind) {
+ case AccessKind::kAttr:
+ os << "." << step->key.cast<ffi::String>();
+ break;
+ case AccessKind::kArrayItem:
+ os << "[" << step->key.cast<int64_t>() << "]";
+ break;
+ case AccessKind::kMapItem:
+ os << "[" << fn_repr(step->key).cast<ffi::String>() << "]";
+ break;
+ case AccessKind::kAttrMissing:
+ os << "[<missing:" << fn_repr(step->key).cast<ffi::String>() <<
">]";
+ break;
+ case AccessKind::kArrayItemMissing:
+ os << "[<missing:" << step->key.cast<int64_t>() << ">]";
+ break;
+ case AccessKind::kMapItemMissing:
+ os << "[<missing:" << fn_repr(step->key).cast<ffi::String>() <<
">]";
+ break;
+ }
+ return os.str();
+ });
+}
+
+inline void AccessPathRegisterRepr() {
+ // Register __ffi_repr__ for AccessPath: produce a concise human-readable
path expression.
+ // Root (no step) emits "<root>"; each non-root node prepends its parent's
repr and appends
+ // the current step's repr. Recursion is delegated to fn_repr so
cycle/cache handling is
+ // owned by the printer.
+ namespace refl = tvm::ffi::reflection;
+ refl::TypeAttrDef<AccessPathObj>().def(refl::type_attr::kRepr,
+ [](AccessPath path, ffi::Function
fn_repr) -> ffi::String {
+ if (!path->step.has_value()) {
+ // Root node: no parent, no step.
+ return "<root>";
+ }
+ std::ostringstream os;
+ os <<
fn_repr(path->parent.value()).cast<ffi::String>();
+ os <<
fn_repr(path->step.value()).cast<ffi::String>();
+ return os.str();
Review Comment:
To ensure robust defensive programming, we should check if `path->parent`
has a value before calling `.value()`. Although in a well-formed path every
non-root node should have a parent, checking `.has_value()` prevents potential
crashes or undefined behavior if a malformed `AccessPathObj` is ever
constructed or passed through the FFI.
```c
if (!path->step.has_value()) {
// Root node: no parent, no
step.
return "<root>";
}
std::ostringstream os;
if (path->parent.has_value()) {
os <<
fn_repr(path->parent.value()).cast<ffi::String>();
}
os <<
fn_repr(path->step.value()).cast<ffi::String>();
return os.str();
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
