8000 Add truncate to array and join/drain/flatten to deque by ethereal-dream · Pull Request #2191 · moonbitlang/core · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

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

Merged
merged 51 commits into from
Jun 26, 2025

Conversation

ethereal-dream
Copy link
Contributor

No description provided.

Copy link
peter-jerry-ye-code-review bot commented May 31, 2025
Missing guard condition return value in UninitializedArray::blit

Category
Correctness
Code Snippet
guard len >= 0 && dst_offset >= 0 && src_offset >= 0 && dst_offset <= dst.length() && src_offset + len <= self.length()
Recommendation
Add an else clause to return the original dst array:

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>

@coveralls
Copy link
Collaborator
coveralls commented May 31, 2025

Pull Request Test Coverage Report for Build 68

Details

  • 0 of 8 (0.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.2%) to 89.003%

Changes Missing Coverage Covered Lines Changed/Added Lines %
builtin/uninitialized_array.mbt 0 8 0.0%
Totals Coverage Status
Change from base Build 62: -0.2%
Covered Lines: 3464
Relevant Lines: 3892

💛 - Coveralls

Copy link
Contributor
@bobzhang bobzhang left a 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

@ethereal-dream ethereal-dream requested a review from bobzhang June 4, 2025 09:13
@ethereal-dream
Copy link
Contributor Author

@bobzhang,I just wanted to check if there are any unresolved issues with this PR.

Copy link
Collaborator
@peter-jerry-ye peter-jerry-ye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you.

@peter-jerry-ye peter-jerry-ye enabled auto-merge (squash) June 20, 2025 02:00
@peter-jerry-ye peter-jerry-ye disabled auto-merge June 20, 2025 02:02
Co-authored-by: Zihang Ye <jerryzihye@outlook.com>
@peter-jerry-ye
Copy link
Collaborator

I just need more tests and verify the implementation of blit

Copy link
Collaborator
@peter-jerry-ye peter-jerry-ye left a 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.

@ethereal-dream
Copy link
Contributor Author

You suggested changing the parameter names from begin/end to start/len, so I updated the corresponding parameter names in the function.

@peter-jerry-ye peter-jerry-ye enabled auto-merge (squash) June 26, 2025 11:13
@peter-jerry-ye peter-jerry-ye dismissed bobzhang’s stale review June 26, 2025 11:13

Discussed offline.

@peter-jerry-ye peter-jerry-ye merged commit 7660b82 into moonbitlang:main Jun 26, 2025
10 checks passed
@peter-jerry-ye
Copy link
Collaborator

Thank you.

peter-jerry-ye added a commit that referenced this pull request Jun 27, 2025
peter-jerry-ye added a commit that referenced this pull request Jun 27, 2025
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.

4 participants
0