Skip to content
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

add a fastpath for grow_if_necessary in stringbuilder_buffer #1408

Merged
merged 1 commit into from
Jan 6, 2025

Conversation

Yu-zh
Copy link
Collaborator

@Yu-zh Yu-zh commented Jan 2, 2025

Converted to draft for now since I think there a potential issue with integer overflow. We should have something like maximum array/string size.

Copy link

peter-jerry-ye-code-review bot commented Jan 2, 2025

‼️ This code review is generated by a bot. Please verify the content before trusting it.

Here are some observations and potential issues from the provided git diff output:

  1. Unnecessary Comment:

    • The comment // current_len is at least 1 might be misleading or unnecessary. The code does not explicitly ensure that current_len is at least 1. If current_len is 0, the logic still works, but the comment could confuse readers. Consider removing or clarifying it.
  2. Redundant Check Removed:

    • The check if enough_space != self.data.length() was removed. This check was likely added to avoid unnecessary reallocation and copying of the array if the space was already sufficient. Removing it might lead to unnecessary memory allocations and copying, which could impact performance. Consider whether this optimization is still needed.
  3. Potential Overflow Risk:

    • The line enough_space = enough_space * 2 could potentially cause an integer overflow if enough_space becomes too large. While this might be rare, it's worth considering adding a check to ensure enough_space does not exceed the maximum allowed size for the array.

These are the key points to consider for improving the code. Let me know if you need further clarification!

@Yu-zh Yu-zh requested a review from bobzhang January 2, 2025 06:56
@coveralls
Copy link
Collaborator

coveralls commented Jan 2, 2025

Pull Request Test Coverage Report for Build 4524

Details

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.01%) to 83.236%

Totals Coverage Status
Change from base Build 4520: 0.01%
Covered Lines: 4717
Relevant Lines: 5667

💛 - Coveralls

@Yu-zh Yu-zh marked this pull request as draft January 2, 2025 08:35
@Yu-zh Yu-zh force-pushed the improve-stringbuffer branch from 2917ec0 to 58a5bf9 Compare January 3, 2025 02:49
@Yu-zh Yu-zh marked this pull request as ready for review January 3, 2025 03:18
@bobzhang bobzhang force-pushed the improve-stringbuffer branch from 58a5bf9 to 31d4205 Compare January 6, 2025 01:12
@bobzhang bobzhang merged commit 3ad9992 into moonbitlang:main Jan 6, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants