-
Notifications
You must be signed in to change notification settings - Fork 1
Add string_truncate utility function #328
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Add string_truncate utility function #328
Conversation
Co-Authored-By: Jake Cosme <jake@cognition.ai>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
| // Truncate a string to a maximum length, adding ellipsis if truncated | ||
| std::string string_truncate(const std::string & str, size_t max_len); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
API Consistency Issue: The function signature uses const std::string & while similar string utility functions in this same file use std::string_view:
// Existing functions use string_view:
bool string_ends_with(const std::string_view & str, const std::string_view & suffix);
bool string_remove_suffix(std::string & str, const std::string_view & suffix);
size_t string_find_partial_stop(const std::string_view & str, const std::string_view & stop);
// New function uses string:
std::string string_truncate(const std::string & str, size_t max_len);Recommendation: For consistency with the existing codebase conventions and to avoid forcing callers to materialize a std::string when they have a string_view or string literal, consider changing the signature to:
std::string string_truncate(std::string_view str, size_t max_len);Additionally, since the PR description already notes the UTF-8 truncation concern, it would be helpful to clarify in the doc comment that max_len is a byte count, not a character count. For example:
// Truncate a string to a maximum byte length, adding ellipsis if truncatedThis makes the behavior explicit and avoids surprising callers who might expect character-based truncation.
Add comment to test if changes are detected
|
Devin - add something similar to docstrings for this change so we can understand what was added |
Co-Authored-By: Jake Cosme <jake@cognition.ai>
|
Done! I've added Doxygen-style documentation for the
See commit c5e26d4. |
| @@ -1,3 +1,5 @@ | |||
| ##testing if devin picks up changes | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Build Blocker: This line is not valid C++ syntax and will cause compilation to fail with an error like stray '##' in program.
This appears to be a test comment that should be removed before merging. If you intended to add a comment, use // for single-line comments:
// testing if devin picks up changesOr simply remove this line entirely since it seems to be for testing purposes only.
| @@ -1,3 +1,5 @@ | |||
| ##testing if devin picks up changes | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Build Error: This line causes a compilation failure:
/home/ubuntu/repos/llama.cpp/common/common.cpp:1:1: error: stray '##' in program
1 | ##testing if devin picks up changes
| ^~
/home/ubuntu/repos/llama.cpp/common/common.cpp:1:3: error: 'testing' does not name a type
## is not a valid C++ comment syntax. This appears to be a test artifact that must be removed before merge. Use // for single-line comments or /* */ for multi-line comments in C++.
| size_t string_find_partial_stop(const std::string_view & str, const std::string_view & stop); | ||
|
|
||
| // Truncate a string to a maximum length, adding ellipsis if truncated | ||
| std::string string_truncate(const std::string & str, size_t max_len); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Convention: Consider using std::string_view instead of const std::string & for consistency with nearby string utility functions in this file.
For example, string_ends_with and string_find_partial_stop (lines 586-588 in common.h) already use std::string_view:
bool string_ends_with(const std::string_view & str, const std::string_view & suffix);
size_t string_find_partial_stop(const std::string_view & str, const std::string_view & stop);Suggested change:
std::string string_truncate(std::string_view str, size_t max_len);This avoids forcing callers to allocate a std::string just to truncate, and maintains consistency with the existing API style in this module.
Make sure to read the contributing guidelines before submitting a PR
Summary
Adds a new
string_truncateutility function to the common library that truncates strings to a maximum length with ellipsis ("...") appended when truncation occurs.Changes
string_truncate(const std::string & str, size_t max_len)function tocommon/common.cppcommon/common.h@brief,@param,@return,@note, and@exampletagsBehavior
Updates Since Last Revision
Human Review Checklist
##testing if devin picks up changesline at the top ofcommon/common.cppthat appears to be a test artifact and should be removed before merge@note)Link to Devin run: https://app.devin.ai/sessions/5e245aad12d74691a4288601a0c20619
Requested by: Jake Cosme (jake@cognition.ai) (@jakexcosme)