gemini-code-assist[bot] commented on code in PR #383:
URL: https://github.com/apache/tvm-ffi/pull/383#discussion_r2663514395
##########
include/tvm/ffi/string.h:
##########
@@ -611,6 +611,58 @@ class String {
}
}
Review Comment:
For consistency with `std::string`, it's good practice to define a static
`npos` member to represent 'not found'. This improves readability and makes the
interface more familiar to C++ developers.
```c
static constexpr size_t npos = static_cast<size_t>(-1);
```
##########
tests/cpp/test_string.cc:
##########
@@ -445,4 +445,40 @@ TEST(String, StdHash) {
EXPECT_EQ(std::hash<Bytes>()(s3), std::hash<Bytes>()(s4));
}
+TEST(String, Find) {
+ String s{"hello world"};
+ EXPECT_EQ(s.find("world"), 6);
+ EXPECT_EQ(s.find("hello"), 0);
+ EXPECT_EQ(s.find("o"), 4);
+ EXPECT_EQ(s.find("o", 5), 7);
+ EXPECT_EQ(s.find("notfound"), size_t(-1));
+ EXPECT_EQ(s.find(""), 0);
+ EXPECT_EQ(s.find("", 5), 5);
+ EXPECT_EQ(s.find("", 20), size_t(-1));
+
+ String pattern{"world"};
+ EXPECT_EQ(s.find(pattern), 6);
+
+ String empty{""};
+ EXPECT_EQ(empty.find("x"), size_t(-1));
+ EXPECT_EQ(empty.find(""), 0);
Review Comment:
To improve test clarity and align with the suggested changes for
`String::find`:
1. Use `String::npos` (which should be added to the `String` class) instead
of `size_t(-1)` for better readability.
2. The test for finding an empty string with a starting position beyond the
string's length should expect `s.size()` to be returned, which is the standard
behavior for `std::string::find`.
```c
EXPECT_EQ(s.find("notfound"), String::npos);
EXPECT_EQ(s.find(""), 0);
EXPECT_EQ(s.find("", 5), 5);
EXPECT_EQ(s.find("", 20), s.size());
String pattern{"world"};
EXPECT_EQ(s.find(pattern), 6);
String empty{"
```
##########
tests/python/test_string.py:
##########
@@ -53,3 +53,18 @@ def test_bytes() -> None:
b5 = pickle.loads(pickle.dumps(b))
assert b5 == b"hello"
assert isinstance(b5, tvm_ffi.core.Bytes)
+
+
+def test_string_find_substr() -> None:
+ s = tvm_ffi.core.String("hello world")
+ assert s.find("world") == 6
+ assert s.find("hello") == 0
+ assert s.find("o") == 4
+ assert s.find("o", 5) == 7
+ assert s.find("notfound") == -1
+ assert s.find("") == 0
Review Comment:
The Python tests for `find` could be more comprehensive. It would be good to
add tests for searching for an empty string with a starting position,
especially one that is out of bounds, to ensure it matches Python's `str.find`
behavior.
For example, Python's `"abc".find("", 5)` returns `3` (the length of the
string).
```suggestion
assert s.find("") == 0
assert s.find("", 5) == 5
assert s.find("", 11) == 11
assert s.find("", 20) == 11
```
##########
include/tvm/ffi/string.h:
##########
@@ -611,6 +611,58 @@ class String {
}
}
+ /*!
+ * \brief Find the first occurrence of a substring
+ * \param str The substring to search for
+ * \param pos The position at which to start the search
+ * \return The position of the first character of the first match, or
size_t(-1) if not found
+ */
+ size_t find(const String& str, size_t pos = 0) const { return
find(str.data(), pos, str.size()); }
+
+ /*!
+ * \brief Find the first occurrence of a substring
+ * \param str The substring to search for
+ * \param pos The position at which to start the search
+ * \return The position of the first character of the first match, or
size_t(-1) if not found
+ */
+ size_t find(const char* str, size_t pos = 0) const { return find(str, pos,
std::strlen(str)); }
+
+ /*!
+ * \brief Find the first occurrence of a substring
+ * \param str The substring to search for
+ * \param pos The position at which to start the search
+ * \param count The length of the substring
+ * \return The position of the first character of the first match, or
size_t(-1) if not found
+ */
+ size_t find(const char* str, size_t pos, size_t count) const {
+ if (count == 0) return pos <= size() ? pos : size_t(-1);
+ if (pos >= size() || count > size() - pos) return size_t(-1);
+
+ const char* this_data = data();
+ size_t this_size = size();
+
+ for (size_t i = pos; i <= this_size - count; ++i) {
+ if (std::memcmp(this_data + i, str, count) == 0) {
+ return i;
+ }
+ }
+ return size_t(-1);
+ }
Review Comment:
The manual implementation of `find` is complex and has a subtle
inconsistency with `std::string::find`'s behavior for searching for empty
strings. It's also likely less performant than the standard library's
implementation.
It's better to delegate to `std::string_view::find`, which is highly
optimized and guaranteed to be correct. This simplifies the code, improves
maintainability, and ensures standard-compliant behavior (including edge cases
like searching for an empty string past the end).
Note that `std::string_view::find` returns `std::string_view::npos` on
failure, which is equivalent to `static_cast<size_t>(-1)`.
```c
size_t find(const char* str, size_t pos, size_t count) const {
return std::string_view(data(), size()).find(std::string_view(str,
count), pos);
}
```
--
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]
