gemini-code-assist[bot] commented on code in PR #377:
URL: https://github.com/apache/tvm-ffi/pull/377#discussion_r2657014269
##########
src/ffi/container.cc:
##########
@@ -70,6 +70,16 @@ TVM_FFI_STATIC_INIT_BLOCK() {
.def("ffi.ArrayGetItem", [](const ffi::ArrayObj* n, int64_t i) -> Any {
return n->at(i); })
.def("ffi.ArraySize",
[](const ffi::ArrayObj* n) -> int64_t { return
static_cast<int64_t>(n->size()); })
+ .def("ffi.ArrayContains",
+ [](const ffi::ArrayObj* n, const Any& value) -> bool {
+ AnyEqual eq;
+ for (const Any& elem : *n) {
+ if (eq(elem, value)) {
+ return true;
+ }
+ }
+ return false;
+ })
Review Comment:
The implementation of `ffi.ArrayContains` is correct, but it can be made
more concise and idiomatic by using `std::any_of` from the `<algorithm>`
header. This improves readability and aligns with modern C++ practices. The
`<algorithm>` header is already available through existing includes.
```c
.def("ffi.ArrayContains",
[](const ffi::ArrayObj* n, const Any& value) -> bool {
AnyEqual eq;
return std::any_of(n->begin(), n->end(),
[&](const Any& elem) { return eq(elem,
value); });
})
```
##########
tests/cpp/test_array.cc:
##########
@@ -292,4 +293,51 @@ TEST(Array, Upcast) {
static_assert(details::type_contains_v<Any, Array<float>>);
}
+TEST(Array, Contains) {
+ Array<int> arr = {1, 2, 3, 4, 5};
+ AnyEqual eq;
+
+ // Test element is present
+ bool found = false;
+ for (const auto& elem : *arr.GetArrayObj()) {
+ if (eq(elem, Any(3))) {
+ found = true;
+ break;
+ }
+ }
+ EXPECT_TRUE(found);
+
+ // Test element is not present
+ found = false;
+ for (const auto& elem : *arr.GetArrayObj()) {
+ if (eq(elem, Any(10))) {
+ found = true;
+ break;
+ }
+ }
+ EXPECT_FALSE(found);
+
+ // Test empty array
+ Array<int> empty_arr;
+ found = false;
+ for (const auto& elem : *empty_arr.GetArrayObj()) {
+ if (eq(elem, Any(1))) {
+ found = true;
+ break;
+ }
+ }
+ EXPECT_FALSE(found);
+
+ // Test with strings
+ Array<String> str_arr = {String("hello"), String("world")};
+ found = false;
+ for (const auto& elem : *str_arr.GetArrayObj()) {
+ if (eq(elem, Any(String("world")))) {
+ found = true;
+ break;
+ }
+ }
+ EXPECT_TRUE(found);
+}
Review Comment:
The `TEST(Array, Contains)` test case currently re-implements the search
logic manually. This means it's testing the search algorithm itself, rather
than verifying that the `ffi.ArrayContains` function is correctly registered
and behaves as expected.
To make this a more effective integration test, you should fetch the
registered `ffi.ArrayContains` function from the FFI registry and call it
directly. This will ensure the entire FFI pathway is working correctly.
Additionally, the test can be made more concise and readable by removing the
repetitive `found` variable and manual loops.
```c
TEST(Array, Contains) {
Function f = Function::Get("ffi.ArrayContains");
ASSERT_TRUE(f.defined());
Array<int> arr = {1, 2, 3, 4, 5};
EXPECT_TRUE(f(arr, 3));
EXPECT_TRUE(f(arr, 1));
EXPECT_TRUE(f(arr, 5));
EXPECT_FALSE(f(arr, 10));
EXPECT_FALSE(f(arr, 0));
Array<int> empty_arr;
EXPECT_FALSE(f(empty_arr, 1));
Array<String> str_arr = {String("hello"), String("world")};
EXPECT_TRUE(f(str_arr, String("hello")));
EXPECT_TRUE(f(str_arr, String("world")));
EXPECT_FALSE(f(str_arr, String("foo")));
}
```
--
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]
