gemini-code-assist[bot] commented on code in PR #18683:
URL: https://github.com/apache/tvm/pull/18683#discussion_r2720205386
##########
web/src/rpc_server.ts:
##########
@@ -234,8 +234,15 @@ export class RPCServer {
if (typeIndex === TypeIndex.kTVMFFIRawStr) {
const str = Uint8ArrayToString(reader.readByteArray());
args.push(str);
+ } else if (typeIndex === TypeIndex.kTVMFFIStr) {
+ reader.readU32(); // skip duplicate type_index
+ const str = Uint8ArrayToString(reader.readByteArray());
+ args.push(str);
} else if (typeIndex === TypeIndex.kTVMFFIByteArrayPtr) {
args.push(reader.readByteArray());
+ } else if (typeIndex === TypeIndex.kTVMFFIBytes) {
+ reader.readU32(); // skip duplicate type_index
+ args.push(reader.readByteArray());
Review Comment:
Similar to the handling of `kTVMFFIStr`, this code reads and discards the
duplicate `type_index` before reading the byte array. It's important to verify
that the C++ side is writing the `type_index` twice for `kTVMFFIBytes` as well.
If the C++ side changes, this code will break.
```suggestion
reader.readU32(); // skip duplicate type_index
args.push(reader.readByteArray());
```
##########
web/src/rpc_server.ts:
##########
@@ -234,8 +234,15 @@ export class RPCServer {
if (typeIndex === TypeIndex.kTVMFFIRawStr) {
const str = Uint8ArrayToString(reader.readByteArray());
args.push(str);
+ } else if (typeIndex === TypeIndex.kTVMFFIStr) {
+ reader.readU32(); // skip duplicate type_index
+ const str = Uint8ArrayToString(reader.readByteArray());
+ args.push(str);
} else if (typeIndex === TypeIndex.kTVMFFIByteArrayPtr) {
args.push(reader.readByteArray());
+ } else if (typeIndex === TypeIndex.kTVMFFIBytes) {
+ reader.readU32(); // skip duplicate type_index
+ args.push(reader.readByteArray());
} else {
throw new Error("cannot support type index " + typeIndex);
}
Review Comment:
The error message "cannot support type index" is too generic. It would be
helpful to include the actual `typeIndex` value in the error message to aid
debugging. This will make it easier to identify unsupported type indices when
they occur.
```suggestion
throw new Error("cannot support type index " + typeIndex + "
(value: " + typeIndex + ")");
```
##########
web/src/rpc_server.ts:
##########
@@ -234,8 +234,15 @@ export class RPCServer {
if (typeIndex === TypeIndex.kTVMFFIRawStr) {
const str = Uint8ArrayToString(reader.readByteArray());
args.push(str);
+ } else if (typeIndex === TypeIndex.kTVMFFIStr) {
+ reader.readU32(); // skip duplicate type_index
+ const str = Uint8ArrayToString(reader.readByteArray());
+ args.push(str);
Review Comment:
The code reads and discards the duplicate `type_index` before reading the
actual string data. This seems correct, but it's crucial to ensure that the C++
side (`WriteFFIAny`) is indeed writing the `type_index` twice for these
specific types. If the C++ side changes, this code will break.
```suggestion
reader.readU32(); // skip duplicate type_index
const str = Uint8ArrayToString(reader.readByteArray());
args.push(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]
