gemini-code-assist[bot] commented on code in PR #674:
URL: https://github.com/apache/tvm-ffi/pull/674#discussion_r3607514974
##########
include/tvm/ffi/reflection/accessor.h:
##########
@@ -207,7 +208,8 @@ inline const TVMFFIMethodInfo*
GetMethodInfo(std::string_view type_key, const ch
TVM_FFI_CHECK_SAFE_CALL(TVMFFITypeKeyToIndex(&type_key_array, &type_index));
const TypeInfo* info = TVMFFIGetTypeInfo(type_index);
for (int32_t i = 0; i < info->num_methods; ++i) {
- if (std::strncmp(info->methods[i].name.data, method_name,
info->methods[i].name.size) == 0) {
+ if (std::strncmp(info->methods[i].name.data, method_name,
info->methods[i].name.size) == 0 &&
+ method_name[info->methods[i].name.size] == '\0') {
return &(info->methods[i]);
}
}
Review Comment:
Using `std::string_view` for string comparison here is cleaner, safer, and
more efficient than manual `std::strncmp` combined with null-termination checks.
### Why this is an improvement:
1. **Performance**: `std::string_view::operator==` first compares the
lengths of the strings. If they do not match, it immediately returns `false`
without performing any character comparisons. In contrast, `std::strncmp` will
compare characters up to the method name's size even if the input `method_name`
is much longer.
2. **Readability**: It avoids manual pointer arithmetic and null-termination
checks (`method_name[info->methods[i].name.size] == '\0'`), making the code
more idiomatic and easier to maintain.
```suggestion
std::string_view target_name(method_name);
for (int32_t i = 0; i < info->num_methods; ++i) {
if (std::string_view(info->methods[i].name.data,
info->methods[i].name.size) == target_name) {
return &(info->methods[i]);
}
}
```
##########
include/tvm/ffi/reflection/accessor.h:
##########
@@ -43,7 +43,8 @@ inline const TVMFFIFieldInfo* GetFieldInfo(std::string_view
type_key, const char
TVM_FFI_CHECK_SAFE_CALL(TVMFFITypeKeyToIndex(&type_key_array, &type_index));
const TypeInfo* info = TVMFFIGetTypeInfo(type_index);
for (int32_t i = 0; i < info->num_fields; ++i) {
- if (std::strncmp(info->fields[i].name.data, field_name,
info->fields[i].name.size) == 0) {
+ if (std::strncmp(info->fields[i].name.data, field_name,
info->fields[i].name.size) == 0 &&
+ field_name[info->fields[i].name.size] == '\0') {
return &(info->fields[i]);
}
}
Review Comment:
Using `std::string_view` for string comparison here is cleaner, safer, and
more efficient than manual `std::strncmp` combined with null-termination checks.
### Why this is an improvement:
1. **Performance**: `std::string_view::operator==` first compares the
lengths of the strings. If they do not match, it immediately returns `false`
without performing any character comparisons. In contrast, `std::strncmp` will
compare characters up to the field name's size even if the input `field_name`
is much longer.
2. **Readability**: It avoids manual pointer arithmetic and null-termination
checks (`field_name[info->fields[i].name.size] == '\0'`), making the code more
idiomatic and easier to maintain.
```suggestion
std::string_view target_name(field_name);
for (int32_t i = 0; i < info->num_fields; ++i) {
if (std::string_view(info->fields[i].name.data,
info->fields[i].name.size) == target_name) {
return &(info->fields[i]);
}
}
```
--
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]
