Skip to content

Conversation

@illusory0x0
Copy link
Contributor

if dst_offset + len > dst.length(), dest array can automatically grow.

Warning: if dst_offset > dst_length(), use unsafe_grow_to_length would create uninitialized element.

@peter-jerry-ye-code-review
Copy link

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

Documentation update may be incomplete regarding array growth behavior

Category
Correctness
Code Snippet
/// * dst_offset exceeds the length of destination array
Recommendation
Add explicit documentation about the auto-growth behavior: '/// * dst_offset exceeds the length of destination array (note: array will automatically grow if dst_offset + len > dst.length())'
Reasoning
The current documentation update removes the length restriction but doesn't explicitly state the new auto-growth behavior. This could lead to confusion about the method's actual behavior.

Missing documentation about uninitialized elements risk

Category
Correctness
Code Snippet
pub fn Array::blit_to[A](
Recommendation
Add a warning in the documentation: '/// Warning: If dst_offset > dst.length(), elements between dst.length() and dst_offset will be uninitialized'
Reasoning
The PR message mentions an important safety consideration about uninitialized elements that should be documented to prevent subtle bugs

Method's safety guarantees could be clearer

Category
Maintainability
Code Snippet
pub fn Array::blit_to[A](
Recommendation
Consider adding @throws or similar annotation to document the exact conditions when the method will fail vs when it will grow the array
Reasoning
For a public API, it's important to be explicit about error conditions vs automatic handling to help users make informed decisions about error handling

@coveralls
Copy link
Collaborator

coveralls commented May 19, 2025

Pull Request Test Coverage Report for Build 6814

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 93.018%

Totals Coverage Status
Change from base Build 6813: 0.0%
Covered Lines: 6222
Relevant Lines: 6689

💛 - Coveralls

@peter-jerry-ye peter-jerry-ye requested a review from tonyfettes May 20, 2025 02:49
if `dst_offset + len > dst.length()`, dest array can automatically grow.

Warning: if `dst_offset > dst_length()`, use `unsafe_grow_to_length` would create uninitialized element.
@tonyfettes tonyfettes force-pushed the fix-docs-Array_blit_to branch from 6c23fd8 to 42c43ff Compare May 20, 2025 02:51
@peter-jerry-ye peter-jerry-ye merged commit 3688c68 into moonbitlang:main May 20, 2025
8 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.

4 participants