8000 Optimize string operations by bobzhang · Pull Request #2253 · moonbitlang/core · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Optimize string operations #2253

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 11, 2025
Merged

Optimize string operations #2253

merged 1 commit into from
Jun 11, 2025

Conversation

bobzhang
Copy link
Contributor

Summary

  • avoid creating intermediate strings by using write_substring
  • remove unused TODO comments in string methods
  • update array joining helpers to directly append views

Testing

  • moon fmt
  • moon info

https://chatgpt.com/codex/tasks/task_e_6849936bc8548320a98083b692ec4327

@peter-jerry-ye-code-review peter-jerry-ye code review
Copy link
Remaining intermediate string allocations in replace_all functions

Category
Performance
Code Snippet
let new = new.to_string()
// use write_substring to avoid intermediate allocations
Recommendation
Consider using write_substring for the 'new' parameter as well to avoid the intermediate allocation
Reasoning
The optimization to avoid intermediate string allocations is applied to many parts of the code, but the 'new' parameter is still converted to a string. This creates an unnecessary allocation that could be avoided using the same write_substring pattern used elsewhere.

Repeated TODO comment about non-ASCII characters across multiple methods

Category
Maintainability
Code Snippet
// TODO: deal with non-ascii characters
guard self.find_by(_.is_ascii_uppercase())
Recommendation
Consider creating a tracking issue for Unicode support and reference it in the comments, or document the current ASCII-only limitation in the function documentation
Reasoning
The TODO comments about non-ASCII characters appear in both to_upper and to_lower methods. This indicates a larger architectural decision about Unicode support that should be tracked and documented more formally.

Duplicated code between String and View implementations

Category
Maintainability
Code Snippet
pub fn View::to_lower(self : View) -> View {
// vs
pub fn to_lower(self : String) -> String {
Recommendation
Consider implementing the String version in terms of the View version to reduce code duplication
Reasoning
The implementations of to_lower, to_upper, and replace_all are nearly identical between String and View types. This duplication makes maintenance more difficult and increases the chance of bugs when modifying the logic.

@bobzhang bobzhang merged commit af1c701 into main Jun 11, 2025
8 of 16 checks passed
@bobzhang bobzhang deleted the codex/find-performance-improvement-opportunities branch June 11, 2025 15:33
@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 7226

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 28 of 28 (100.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.01%) to 93.111%
Totals Coverage Status
Change from base Build 7224: 0.01%
Covered Lines: 8569
Relevant Lines: 9203
💛 - Coveralls

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
codex
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bobzhang @coveralls
0