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

add int16 and uint16 operators #1421

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

add int16 and uint16 operators #1421

wants to merge 1 commit into from

Conversation

Yu-zh
Copy link
Collaborator

@Yu-zh Yu-zh commented Jan 3, 2025

No description provided.

Copy link

peter-jerry-ye-code-review bot commented Jan 3, 2025

‼️ This code review is generated by a bot. Please verify the content before trusting it.

Based on the provided git diff, here are three potential issues or observations:

  1. Inconsistent Method Naming in Int16 and UInt16 Implementations:

    • In the Int16 implementation, the method to_int16 is used, while in the UInt16 implementation, the method to_uint16 is used. This inconsistency in naming conventions could lead to confusion or errors when developers are working with these types. It would be better to ensure that the naming conventions are consistent across similar types.
  2. Potential Overflow/Underflow Issues in Arithmetic Operations:

    • The arithmetic operations (op_add, op_sub, op_mul, op_div) in both Int16 and UInt16 implementations rely on intermediate conversions to Int before converting back to the respective type. This could lead to overflow or underflow issues, especially since Int has a larger range than Int16 or UInt16. For example, in Int16::op_add, adding two large Int16 values could result in an intermediate Int value that exceeds the range of Int16, leading to incorrect results when converting back. The same issue applies to UInt16.
  3. Missing Tests for Edge Cases:

    • While the test files (int16_test.mbt and uint16_test.mbt) are comprehensive, there are some edge cases that are not covered. For example:
      • In Int16, there are no tests for the behavior of op_div when dividing by zero, which should ideally throw an error or handle it gracefully.
      • In UInt16, there are no tests for the behavior of op_mul when multiplying two large numbers that would cause an overflow, such as 65535 * 65535. The expected behavior should be clearly defined and tested.

These issues should be addressed to ensure the robustness and reliability of the code.

@coveralls
Copy link
Collaborator

coveralls commented Jan 3, 2025

Pull Request Test Coverage Report for Build 4587

Details

  • 14 of 20 (70.0%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.05%) to 83.248%

Changes Missing Coverage Covered Lines Changed/Added Lines %
int16/int16.mbt 7 8 87.5%
uint16/uint16.mbt 7 8 87.5%
builtin/console.mbt 0 2 0.0%
builtin/show.mbt 0 2 0.0%
Totals Coverage Status
Change from base Build 4586: -0.05%
Covered Lines: 4741
Relevant Lines: 5695

💛 - 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