Skip to content
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

feat: rename to_int64 to reinterpret_as_int64 #1128

Merged
merged 2 commits into from
Oct 16, 2024

Conversation

qazxcdswe123
Copy link
Contributor

No description provided.

Copy link

peter-jerry-ye-code-review bot commented Oct 15, 2024

From the provided git diff output, I observe several issues that could potentially affect the correctness and maintainability of the code. Here are three notable points:

  1. Deprecation of to_int64 in Favor of reinterpret_as_int64:

    • Multiple files, such as builtin/int64_js.mbt, builtin/int64_nonjs.mbt, and others, include changes where the to_int64 function is being deprecated in favor of reinterpret_as_int64. This indicates a shift in the API or the intended way to convert between types.
    • Example:
      +pub fn UInt64::reinterpret_as_int64(self : UInt64) -> Int64 = "%identity"
      -pub fn UInt64::to_int64(self : UInt64) -> Int64 = "%identity"
    • Suggested Action: Update all instances in the codebase where to_int64 is used to use reinterpret_as_int64 instead. This ensures consistency and avoids future deprecation warnings or errors.
  2. Potential Typo in Function Name:

    • In builtin/bigint.mbt, there's a new function reinterpret_as_int64 that appears similar to the existing to_int64. The naming convention might be intended to clarify the operation but could lead to confusion if not documented properly.
    • Example:
      +reinterpret_as_int64()
      -to_int64()
    • Suggested Action: Ensure that the documentation and comments in the codebase clearly explain the difference between reinterpret_as_int64 and to_int64 (if applicable) to avoid confusion among developers.
  3. Use of .to_int64().reinterpret_as_uint64():

    • In builtin/bigint.mbt, there are instances where the code performs a conversion from UInt64 to Int64 and then back to UInt64. This might be redundant and could be simplified.
    • Example:
      -limbs[i] = (borrow & radix_mask.to_int64()).reinterpret_as_uint64()
      +limbs[i] = (borrow & radix_mask.reinterpret_as_int64()).reinterpret_as_uint64()
    • Suggested Action: Review the code logic to ensure that the conversion is necessary. If the intent is to perform a bitwise operation, consider whether the direct use of UInt64 would suffice, avoiding unnecessary type conversions.

These observations highlight potential areas for improvement, including updating deprecated functions, clarifying documentation, and reviewing unnecessary type conversions. Addressing these points can enhance code clarity, maintainability, and performance.

@coveralls
Copy link
Collaborator

coveralls commented Oct 15, 2024

Pull Request Test Coverage Report for Build 3576

Details

  • 12 of 13 (92.31%) changed or added relevant lines in 6 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.1%) to 82.319%

Changes Missing Coverage Covered Lines Changed/Added Lines %
builtin/bigint.mbt 5 6 83.33%
Totals Coverage Status
Change from base Build 3575: 0.1%
Covered Lines: 4195
Relevant Lines: 5096

💛 - Coveralls

@qazxcdswe123 qazxcdswe123 force-pushed the to-int64-to-reinterpret-as-int64 branch 3 times, most recently from d8be8bd to 4615da4 Compare October 15, 2024 08:12
int64/xxhash.mbt Outdated Show resolved Hide resolved
@qazxcdswe123 qazxcdswe123 force-pushed the to-int64-to-reinterpret-as-int64 branch from 52e1e1b to 514a5d7 Compare October 16, 2024 01:48
@qazxcdswe123 qazxcdswe123 merged commit c24f901 into main Oct 16, 2024
12 checks passed
@qazxcdswe123 qazxcdswe123 deleted the to-int64-to-reinterpret-as-int64 branch October 16, 2024 01:51
tonyfettes pushed a commit to tonyfettes/moonbitlang-core that referenced this pull request Oct 16, 2024
* feat: rename to_int64 to reinterpret_as_int64

* fix: better type convertion
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