Skip to content

Conversation

peter-jerry-ye
Copy link
Collaborator

No description provided.

Copy link

Potential overflow in upper bound check for to_char method

Category
Correctness
Code Snippet
if self is (0..=0xD7FF) || self is (0xE000..<_) {
Some(self.unsafe_to_char())
} else {
None
}
Recommendation
Use explicit upper bound check: if self is (0..=0xD7FF) || self is (0xE000..=0xFFFF) { or validate that UInt16 maximum value is 0xFFFF
Reasoning
The 0xE000..<_ pattern with underscore might not behave as expected for the upper bound. Since UInt16 has a maximum value of 0xFFFF, it's safer to be explicit about the valid Unicode scalar value range.

Duplicated range constants should be extracted

Category
Maintainability
Code Snippet
self >= 0xD800 && self <= 0xDBFF
self >= 0xDC00 && self <= 0xDFFF
self >= 0xD800 && self <= 0xDFFF
Recommendation
Extract constants: const leading_surrogate_start : UInt16 = 0xD800, const leading_surrogate_end : UInt16 = 0xDBFF, etc., and reuse them across methods
Reasoning
Magic numbers are repeated multiple times across different methods. Extracting them as named constants improves maintainability and reduces the risk of typos in future changes.

Missing documentation for unsafe_to_char method

Category
Maintainability
Code Snippet
///|
pub fn UInt16::unsafe_to_char(self : UInt16) -> Char {
Recommendation
Add comprehensive documentation explaining when it's safe to use and the potential consequences of misuse, similar to other methods in the file
Reasoning
The 'unsafe' prefix indicates this method can cause undefined behavior or incorrect results. Proper documentation is crucial to help developers understand when it's safe to use and what validation they need to perform beforehand.

@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 1188

Details

  • 3 of 7 (42.86%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.03%) to 89.366%

Changes Missing Coverage Covered Lines Changed/Added Lines %
uint16/uint16.mbt 3 7 42.86%
Totals Coverage Status
Change from base Build 1185: -0.03%
Covered Lines: 8841
Relevant Lines: 9893

💛 - Coveralls

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.

2 participants