Skip to content

ArrayView::join and FixedArray::join #2125

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

Merged
merged 9 commits into from
May 28, 2025

Conversation

ethereal-dream
Copy link
Contributor

Adding ArrayView::join and FixedArray::join functions makes it convenient to concatenate strings within an array into a single complete string.

Copy link

peter-jerry-ye-code-review bot commented May 18, 2025

Duplicate implementation of join logic in both View::join and FixedArray::join

Category
Maintainability
Code Snippet
Both View::join and FixedArray::join implement similar string joining logic
Recommendation
Extract the common join logic into a private helper function that can be used by both implementations
Reasoning
The current implementation duplicates the size calculation and string building logic. A shared implementation would reduce code duplication and make future maintenance easier

Size hint calculation could be more accurate

Category
Performance
Code Snippet
size_hint = size_hint << 1 // in View::join
Recommendation
Remove the doubling of size_hint as the calculation already accounts for all string lengths and separators
Reasoning
The current implementation doubles the calculated size hint which may lead to unnecessary memory allocation. The size hint calculation already includes all string lengths and separator lengths

Inconsistent separator handling between empty and non-empty cases

Category
Correctness
Code Snippet
if separator is "" vs if separator.is_empty()
Recommendation
Use consistent empty check with separator.is_empty() across all implementations
Reasoning
View::join uses 'is ""' while FixedArray::join uses 'is_empty()'. Using consistent method calls makes the code more maintainable and prevents potential bugs if string comparison behavior changes

@coveralls
Copy link
Collaborator

coveralls commented May 18, 2025

Pull Request Test Coverage Report for Build 6960

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 92.498%

Totals Coverage Status
Change from base Build 6956: 0.01%
Covered Lines: 8730
Relevant Lines: 9438

💛 - Coveralls

@peter-jerry-ye
Copy link
Collaborator

@Yu-zh asked whether we should be using View instead of String.

I think maybe we can use @string.View for separator, but keep T[String] for this signature.

@ethereal-dream
Copy link
Contributor Author

When using @string.View for separator, what should I do with string.write_string(separator) since there doesn't seem to be a write_view function?

@peter-jerry-ye peter-jerry-ye linked an issue May 22, 2025 that may be closed by this pull request
@peter-jerry-ye
Copy link
Collaborator

I think you can simply call to_string on the view. The support for view on the string builder would come later as its currently blocked by how the core package is organize, which I assume would still need a few weeks.

@peter-jerry-ye peter-jerry-ye enabled auto-merge (squash) May 28, 2025 08:57
@peter-jerry-ye peter-jerry-ye disabled auto-merge May 28, 2025 09:11
@peter-jerry-ye peter-jerry-ye merged commit d8ab654 into moonbitlang:main May 28, 2025
11 of 12 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.

[Feature Request] ArrayView::join and FixedArray::join
3 participants