gemini-code-assist[bot] commented on code in PR #425:
URL: https://github.com/apache/tvm-ffi/pull/425#discussion_r2756007126


##########
python/tvm_ffi/cython/function.pxi:
##########
@@ -918,9 +918,15 @@ cdef class Function(Object):
         # directly inline here to simplify the resulting trace
         if c_api_ret_code == 0:
             return make_ret(result, c_ctx_dlpack_api)
-        elif c_api_ret_code == -2:
-            raise_existing_error()
-        raise move_from_last_error().py_error()
+        # backward compact with error already set case
+        # TODO(tqchen): remove after we move beyond a few versions.
+        if c_api_ret_code == -2:
+            raise raise_existing_error()
+        # epecial handle env error already set
+        error = move_from_last_error()
+        if error.kind == "EnvErrorAlreadySet":
+            raise raise_existing_error()
+        raise error.py_error()

Review Comment:
   ![medium](https://www.gstatic.com/codereviewagent/medium-priority.svg)
   
   This error handling logic is duplicated. To improve maintainability, please 
use the `_raise_if_error` helper function proposed in my comment on 
`python/tvm_ffi/cython/error.pxi`.
   
   This centralizes the error handling, and the code here becomes much cleaner.
   
   ```
           _raise_if_error(c_api_ret_code)
           return make_ret(result, c_ctx_dlpack_api)
   ```



##########
python/tvm_ffi/cython/type_info.pxi:
##########
@@ -55,9 +55,16 @@ cdef class FieldSetter:
         # directly inline here to simplify backtrace
         if c_api_ret_code == 0:
             return
-        elif c_api_ret_code == -2:
-            raise_existing_error()
-        raise move_from_last_error().py_error()
+        # backward compact with error already set case
+        # TODO(tqchen): remove after we move beyond a few versions.
+        if c_api_ret_code == -2:
+            raise raise_existing_error()
+        # epecial handle env error already set
+        error = move_from_last_error()
+        if error.kind == "EnvErrorAlreadySet":
+            raise raise_existing_error()
+        raise error.py_error()

Review Comment:
   ![medium](https://www.gstatic.com/codereviewagent/medium-priority.svg)
   
   This is another instance of the duplicated error handling logic. Please use 
the `_raise_if_error` helper function proposed in my comment on 
`python/tvm_ffi/cython/error.pxi` to improve maintainability.
   
   ```
           _raise_if_error(c_api_ret_code)
           return
   ```



-- 
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]

Reply via email to