Skip to content

Conversation

@bobzhang
Copy link
Contributor

  • add Int::to_char safe API
  • add Int::unsafe_to_char

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

The test 'Int::to_char - Invalid values' is duplicated

Category
Correctness
Code Snippet
test 'Int::to_char - Invalid values' appears twice in intrinsics_test.mbt
Recommendation
Remove one of the duplicate test blocks or rename them to be more specific about what they're testing
Reasoning
Having duplicate test names can cause confusion and makes it harder to reference specific tests. The two test blocks test different invalid cases, so they should have distinct names.

Missing documentation for unsafe_to_char and to_char functions

Category
Maintainability
Code Snippet
///|
pub fn Int::unsafe_to_char(self : Int) -> Char = "%char_from_int"

///|
pub fn Int::to_char(self : Int) -> Char?
Recommendation
Add detailed documentation explaining the valid range of inputs, the purpose of each function, and why/when to use the unsafe version
Reasoning
Functions dealing with character encoding and unsafe operations should be well-documented to prevent misuse and make the API more user-friendly. The documentation should explain the valid Unicode ranges and the risks of using unsafe_to_char.

Missing symbolic constants for Unicode range values

Category
Maintainability
Code Snippet
if self is (0..=0xD7FF) || self is (0xE000..=0x10FFFF)
Recommendation
Define named constants for these Unicode range boundaries:

const UNICODE_HIGH_SURROGATE_START : Int = 0xD800
const UNICODE_LOW_SURROGATE_END : Int = 0xDFFF
const UNICODE_MAX : Int = 0x10FFFF

Reasoning
Using magic numbers makes the code harder to maintain and understand. Named constants would make the code more readable and ensure consistency if these values need to be referenced elsewhere.

@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 6449

Details

  • 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.003%) to 92.686%

Totals Coverage Status
Change from base Build 6448: 0.003%
Covered Lines: 6121
Relevant Lines: 6604

💛 - Coveralls

@bobzhang bobzhang merged commit d911ec4 into main Apr 26, 2025
12 checks passed
@bobzhang bobzhang deleted the hongbo/unsafe_to_char branch April 26, 2025 01:57
@bobzhang
Copy link
Contributor Author

cc @peter-jerry-ye we will use x.(unsafe)_to_char to replace Char::from_int since the former provide a better auto-completion experience

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.

3 participants