-
Notifications
You must be signed in to change notification settings - Fork 126
Add truncate to array and join/drain/flatten to deque #2191
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
Conversation
Missing guard condition return value in UninitializedArray::blitCategory guard len >= 0 &&
dst_offset >= 0 &&
src_offset >= 0 &&
dst_offset <= dst.length() &&
src_offset + len <= self.length() else { return dst }```
**Reasoning**
The guard statement lacks a return value when the conditions fail. Since the function must return a UninitializedArray, we should return the original dst array to maintain consistent behavior.
</details>
<details>
<summary> Potential double allocation in Deque::join implementation </summary>
**Category**
Performance
**Code Snippet**
let str = separator.to_string()
self.iter().join(str)
**Recommendation**
Consider passing the string view directly to avoid unnecessary allocation:
```moonbit
self.iter().join(separator)```
**Reasoning**
Converting the string view to a string before joining creates an unnecessary allocation. The iterator's join operation should be able to work directly with string views.
</details>
<details>
<summary> Complex nested logic in Deque::drain without intermediate variables </summary>
**Category**
Maintainability
**Code Snippet**
ignore(
self.buf.blit(
self.buf,
len=self.length() - end,
src_offset=end,
dst_offset=start,
),
)
**Recommendation**
Break down the complex operation into named intermediate steps:
```moonbit
let remaining_len = self.length() - end
let _ = self.buf.blit(
self.buf,
len=remaining_len,
src_offset=end,
dst_offset=start
)```
**Reasoning**
The current implementation nests multiple operations making it harder to understand and debug. Using intermediate variables improves readability and makes the code's intent clearer.
</details> |
Pull Request Test Coverage Report for Build 68Details
💛 - Coveralls |
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.
we would like to keep each PR scope small, so that we can merge some parts faster when we agree on
Change implementation of deque::join Change truncate from function to array method Change join,drain,flatten from function to deque method
@bobzhang,I just wanted to check if there are any unresolved issues with this PR. |
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.
LGTM. Thank you.
Co-authored-by: Zihang Ye <jerryzihye@outlook.com>
I just need more tests and verify the implementation of |
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.
Why do you change implementation after the approval.
You suggested changing the parameter names from |
Thank you. |
No description provided.