feat: add find and substr methods to String #383
Conversation
Summary of ChangesHello @guan404ming, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds useful find and substr methods to the String class, which is a great enhancement. The implementation of substr is solid. For the find method, I've suggested refactoring it to use std::string_view::find. This will simplify the code, make it more robust by aligning its behavior with the standard library (especially for edge cases like searching for an empty string), and likely improve performance. I've also recommended adding a String::npos static member for better API consistency with std::string. The tests have been updated accordingly, with suggestions to improve coverage for edge cases in both C++ and Python.
| 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); | ||
| } |
There was a problem hiding this 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).
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);
}| throw std::out_of_range("tvm::String index out of bounds"); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this 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.
static constexpr size_t npos = static_cast<size_t>(-1);
tests/cpp/test_string.cc
Outdated
| 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); |
There was a problem hiding this comment.
To improve test clarity and align with the suggested changes for String::find:
- Use
String::npos(which should be added to theStringclass) instead ofsize_t(-1)for better readability. - 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 forstd::string::find.
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{"| assert s.find("o") == 4 | ||
| assert s.find("o", 5) == 7 | ||
| assert s.find("notfound") == -1 | ||
| assert s.find("") == 0 |
There was a problem hiding this 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).
| assert s.find("") == 0 | |
| assert s.find("") == 0 | |
| assert s.find("", 5) == 5 | |
| assert s.find("", 11) == 11 | |
| assert s.find("", 20) == 11 |
|
Thanks! |
2 participants
Why
String class lacks essential string manipulation methods that are commonly expected in standard library equivalents, requiring users to convert to std::string for basic operations like substring search and extraction.
How