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

align BigInt to JavaScript native bigint #1158

Merged
merged 2 commits into from
Oct 23, 2024
Merged

align BigInt to JavaScript native bigint #1158

merged 2 commits into from
Oct 23, 2024

Conversation

hackwaly
Copy link
Contributor

@hackwaly hackwaly commented Oct 22, 2024

This MR split bigint.mbt into three files:

  • bigint_js.mbt: Added js platform specific implementation
  • bigint_nonjs.mbt: Make previous implementation non-js platform specific
  • bigint_deprecated.mbt: Extracted from previous implementation and it is shared

This MR split bigint_wbtest.mbt into three parts:

  • bigint_wbtest.mbt: Previous white-box tests
  • bigint_js_wbtest.mbt: Redefine check_len function used in bigint_wbtest.mbt to make it do nothing
  • bigint_nonjs_wbtest.mbt: Previous check_len function implementation and other tests depends on previous internal representation of BigInt.

@hackwaly hackwaly changed the title align BigInt to javascript native bigint align BigInt to JavaScript native bigint Oct 22, 2024
Copy link

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

Based on the provided git diff output, here are three potential issues or suggestions for improvement:

  1. Deprecation Notices and Use of Bitwise Operators:

    • The files bigint_deprecated.mbt and bigint_nonjs.mbt contain functions like asr, shl, lsl, shr, and asr which are marked with a deprecation alert suggesting the use of infix bitwise operators >> and << instead. This is a good practice to ensure code consistency and to leverage built-in operator overloading capabilities. However, it's important to ensure that all references to these deprecated functions are updated accordingly to avoid future issues.
    • Suggestion: Review and update all codebases that use these deprecated functions to use the recommended infix operators. This will not only comply with the deprecation notices but also make the code more readable and maintainable.
  2. Error Handling in Bitwise Shift Operations:

    • In bigint_nonjs.mbt, the functions op_shl and op_shr check if the shift count n is negative and abort with an error message if it is. This is a good practice to prevent undefined behavior.
    • Suggestion: Consider extending this error handling to include more specific error messages or exceptions that can be caught and handled appropriately by the calling code. This could improve the robustness of the code by allowing for more graceful error recovery.
  3. Code Duplication and Renaming:

    • The file bigint.mbt was renamed to bigint_nonjs.mbt, and similar changes were made to test files and other related files. This indicates a potential for code duplication or inconsistency across different environments (e.g., JS vs. non-JS).
    • Suggestion: Ensure that the logic and functionality are consistent across both environments. If there are significant differences in implementation, consider maintaining a clear separation or abstraction to avoid confusion and ensure that both implementations serve the same purpose effectively.

These suggestions aim to improve code consistency, maintainability, and robustness based on the observed changes in the git diff output.

@coveralls
Copy link
Collaborator

coveralls commented Oct 22, 2024

Pull Request Test Coverage Report for Build 3674

Details

  • 0 of 4 (0.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.4%) to 82.905%

Changes Missing Coverage Covered Lines Changed/Added Lines %
builtin/bigint_deprecated.mbt 0 4 0.0%
Totals Coverage Status
Change from base Build 3673: 0.4%
Covered Lines: 4263
Relevant Lines: 5142

💛 - Coveralls

@bobzhang bobzhang merged commit 6ac8f07 into main Oct 23, 2024
13 checks passed
@bobzhang bobzhang deleted the yuxiang/bigint-js branch October 23, 2024 01:05
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