8000 Add `to_uint()` and `to_uint64()` to `@UInt16` by Asterless · Pull Request #2259 · moonbitlang/core · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Add to_uint() and to_uint64() to @UInt16 #2259

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 13, 2025
Merged

Add to_uint() and to_uint64() to @UInt16 #2259

merged 1 commit into from
Jun 13, 2025

Conversation

Asterless
Copy link
Contributor

Fixes #2214

@peter-jerry-ye-code-review peter-jerry-ye code review
Copy link
peter-jerry-ye-code-review bot commented Jun 12, 2025 •
edited
Loading
Documentation comments are missing for the new methods

Category
Maintainability
Code Snippet
pub fn UInt16::to_uint(self : UInt16) -> UInt
pub fn UInt16::to_uint64(self : UInt16) -> UInt64
Recommendation
Add descriptive documentation comments explaining the purpose and behavior of these conversion methods:

///| Converts a UInt16 value to UInt
pub fn UInt16::to_uint(self : UInt16) -> UInt

///| Converts a UInt16 value to UInt64
pub fn UInt16::to_uint64(self : UInt16) -> UInt64

Reasoning
Documentation helps other developers understand the purpose and usage of these methods. While the method names are somewhat self-explanatory, documenting behavior, especially for type conversions, is important for maintainability.

Test cases could be more comprehensive

Category
Correctness
Code Snippet
test "UInt16::to_uint" {
inspect(UInt16::to_uint(0), content="0")
inspect(UInt16::to_uint(1), content="1")
...
}
Recommendation
Add edge cases and potentially problematic values to the test cases:

test "UInt16::to_uint" {
  // Existing tests
  // Add edge cases
  inspect(UInt16::to_uint(UInt16::max_value), content="65535")
  inspect(UInt16::to_uint(UInt16::min_value), content="0")
}

Reasoning
While the current tests cover basic cases, testing edge cases (minimum and maximum values) explicitly helps ensure the conversion methods handle all possible input values correctly.

Multiple conversions in to_uint implementation

Category
Performance
Code Snippet
pub fn UInt16::to_uint(self : UInt16) -> UInt {
self.to_int().reinterpret_as_uint()
}
Recommendation
Consider implementing a direct conversion if possible:

pub fn UInt16::to_uint(self : UInt16) -> UInt {
  reinterpret_as_uint(self)
}

Reasoning
The current implementation converts UInt16 to Int first, then to UInt. If possible, a direct conversion might be more efficient, though this depends on the language's internal representation and available primitives.

@Asterless Asterless changed the title Add to_uint() and to_uint64 to @UInt16 Add to_uint and to_uint64 to @UInt16 Jun 12, 2025
@Asterless Asterless changed the title Add to_uint and to_uint64 to @UInt16 Add to_uint() and to_uint64() to @UInt16 Jun 12, 2025
@coveralls
Copy link
Collaborator
coveralls commented Jun 12, 2025 •
edited
Loading

Pull Request Test Coverage Report for Build 7261

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.002%) to 93.112%
Totals Coverage Status
Change from base Build 7259: 0.002%
Covered Lines: 8570
Relevant Lines: 9204
💛 - Coveralls

@peter-jerry-ye peter-jerry-ye enabled auto-merge (rebase) June 13, 2025 08:41
@peter-jerry-ye peter-jerry-ye merged commit 5333fa2 into moonbitlang:main Jun 13, 2025
11 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

adding to_uint() and to_uint64() functions to the UInt16 type
3 participants
@Asterless @coveralls @peter-jerry-ye
0