gemini-code-assist[bot] commented on code in PR #353: URL: https://github.com/apache/tvm-ffi/pull/353#discussion_r2637621359
########## examples/stable_c_abi/run_all.bat: ########## @@ -0,0 +1,33 @@ +@echo off +rem Licensed to the Apache Software Foundation (ASF) under one +rem or more contributor license agreements. See the NOTICE file +rem distributed with this work for additional information +rem regarding copyright ownership. The ASF licenses this file +rem to you under the Apache License, Version 2.0 (the +rem "License"); you may not use this file except in compliance +rem with the License. You may obtain a copy of the License at +rem +rem http://www.apache.org/licenses/LICENSE-2.0 +rem +rem Unless required by applicable law or agreed to in writing, +rem software distributed under the License is distributed on an +rem "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +rem KIND, either express or implied. See the License for the +rem specific language governing permissions and limitations +rem under the License. + +setlocal enabledelayedexpansion +cd /d "%~dp0" + +if exist build rmdir /s /q build + +rem To compile `src/add_one_cpu.c` to shared library `build/add_one_cpu.so` +cmake . -B build -DEXAMPLE_NAME="kernel" -DCMAKE_BUILD_TYPE=RelWithDebInfo +cmake --build build --config RelWithDebInfo + +rem To compile `src/load.c` to executable `build/load` +cmake . -B build -DEXAMPLE_NAME="load" -DCMAKE_BUILD_TYPE=RelWithDebInfo +cmake --build build --config RelWithDebInfo Review Comment:  Similar to the other new batch script, running the `cmake` configuration command again here is inefficient. It would be better to configure once and build different targets. This is a pre-existing pattern from the `.sh` script, but for future maintainability, it would be good to refactor the CMake setup to allow for a single configuration step followed by building multiple targets. For example: ```batch rem Configure once cmake . -B build -DCMAKE_BUILD_TYPE=RelWithDebInfo rem Build kernel target cmake --build build --target kernel rem Build load target cmake --build build --target load build\load ``` This would require changes to `CMakeLists.txt` which is out of scope for this PR, but it's a good practice to keep in mind. ########## examples/quickstart/run_all_cpu.bat: ########## @@ -0,0 +1,36 @@ +@echo off +rem Licensed to the Apache Software Foundation (ASF) under one +rem or more contributor license agreements. See the NOTICE file +rem distributed with this work for additional information +rem regarding copyright ownership. The ASF licenses this file +rem to you under the Apache License, Version 2.0 (the +rem "License"); you may not use this file except in compliance +rem with the License. You may obtain a copy of the License at +rem +rem http://www.apache.org/licenses/LICENSE-2.0 +rem +rem Unless required by applicable law or agreed to in writing, +rem software distributed under the License is distributed on an +rem "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +rem KIND, either express or implied. See the License for the +rem specific language governing permissions and limitations +rem under the License. + +setlocal enabledelayedexpansion +cd /d "%~dp0" + +if exist build rmdir /s /q build + +rem To compile `compile/add_one_cpu.cc` to shared library `build/add_one_cpu.so` +cmake . -B build -DEXAMPLE_NAME="compile_cpu" -DCMAKE_BUILD_TYPE=RelWithDebInfo +cmake --build build --config RelWithDebInfo + +rem To load and run `add_one_cpu.so` in NumPy +python load\load_numpy.py + +rem To load and run `add_one_cpu.so` in C++ +cmake . -B build -DEXAMPLE_NAME="load_cpp" -DCMAKE_BUILD_TYPE=RelWithDebInfo +cmake --build build --config RelWithDebInfo Review Comment:  Running the `cmake` configuration command again is inefficient as it can trigger a full project re-configuration. While this works for this small example, for larger projects this can be slow. This pattern is also present in the corresponding `.sh` script and should ideally be addressed in both. A better practice would be to configure once and then build specific targets. For example, if the CMake project was structured to support it, you could do: ```batch rem Configure once cmake . -B build -DCMAKE_BUILD_TYPE=RelWithDebInfo rem Build first target cmake --build build --target compile_cpu rem To load and run `add_one_cpu.so` in NumPy python load\load_numpy.py rem Build second target cmake --build build --target load_cpp build\load_cpp ``` This would require changes to the `CMakeLists.txt` to define these targets. I'm leaving this as a suggestion for future improvement, as it's out of scope for just adding this batch file. ########## examples/packaging/run_example.py: ########## @@ -32,23 +33,22 @@ def run_add_one() -> None: def run_raise_error() -> None: """Invoke raise_error from the extension to demonstrate error handling.""" - my_ffi_extension.raise_error("This is an error") + print("=========== Example 2: raise_error ===========") + try: + my_ffi_extension.raise_error("This is an error") + except: Review Comment:  Using a bare `except:` is discouraged as it catches all exceptions, including system-exiting exceptions like `SystemExit` and `KeyboardInterrupt`, which can hide other problems and make it harder to interrupt the program. It's better to catch a more specific exception, like `Exception`. ```suggestion except Exception: ``` -- 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]
