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

Fix #595 : Numeric operator overflow #596

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

codesplode
Copy link

@codesplode codesplode commented Jun 16, 2021

Initial pull request that just fixes the comparison operators and could be greatly improved by:

  • Merging type specific checks with the already type specific arithmetic operators, applying this fix to all Numeric Binary Expressions.
  • More careful consideration of converting numbers of different types, converting the operands to types that can always appropriately handle both numeric values.

Fixes #595

@codesplode
Copy link
Author

codesplode commented Jun 17, 2021

If a maintainer could review everything, I've completed a significant change to the Binary Comparison and Operation methods in OperatorUtils that should handle all numeric types effectively. When the two operands are of the same class I simplified the type checks.

In all cases, the code only converts a number between types when necessary. For operands of differing types, it converts them to the highest complexity / scale type of the two.

As a bonus, I also added BigInteger support. All unit tests complete successfully, but it may be a good idea to add another unit test similar to the comparison one I have that checks the arithmetic operation logic specifically. 100% code coverage there would not be a bad thing.

@ebussieres ebussieres force-pushed the fix/595-numeric-operator-overflow branch from 8b6f327 to dc32712 Compare October 28, 2022 14:25
/*
* Convert and compare operands based on the complexity / scale of the values.
*/
if (num1 instanceof BigDecimal) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to add tests when both operands are not the same type

Copy link
Member

@ebussieres ebussieres Oct 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should also add tests for numeric operation (add, subtract, multiply, etc.)

@ebussieres ebussieres added this to the 3.2.0 milestone Oct 28, 2022
@ebussieres
Copy link
Member

Great PR, I'll gladly accept it when test coverage is gonna be better. Can you adjust it ?

@ebussieres ebussieres removed this from the 3.2.0 milestone Nov 24, 2022
@thecoden
Copy link

thecoden commented Dec 3, 2022

Hi @ebussieres , I somehow missed your comments but will review and see if I can find some time to add more test cases.

@ArneBab
Copy link

ArneBab commented May 24, 2024

@thecoden did you get to add more test cases?

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.

Long, BigInteger, BigDecimal Operators Fail Due to Conversion to Double
4 participants