Fix problem of unnecessarily widening float values by opeongo · Pull Request #768 · beanshell/beanshell

opeongo · 2024-07-29T19:02:09Z

This is a fix for issue #767 where float expressions are widened to doubles.

nickl-

Needs work.

I'm wondering if there isn't a simpler solution, to use the double functionality as is, but if it is floats we do a shrink (if they fit) else expand to Double/BigDecimal as needed. This will be a much smaller change and less things that can cause additional headaches.

nickl- · 2024-08-10T10:54:52Z

src/main/java/bsh/Operators.java

        if (lhs instanceof BigDecimal)
            return bigDecimalBinaryOperation( (BigDecimal) lhs, (BigDecimal) rhs, kind );
+        if (lhs instanceof Float)
+            return floatBinaryOperation( (Float) lhs, (Float) rhs, kind );


This will cause a precision problem, and even cast error, if rhs is double.

binaryOperationImpl only has one usage (in Operators.binaryOperation()), and that is after lhs and rhs have been passed through Operators.promotePrimitves(). At this point if lhs is Float it is not possible for rhs to not be Float.

nickl- · 2024-08-10T11:04:29Z

src/main/java/bsh/Operators.java

+
+        }
+        if ( OVERFLOW_OPS.contains(kind) )
+            return bigDecimalBinaryOperation(BigDecimal.valueOf(lhs), BigDecimal.valueOf(rhs), kind);


I wonder if we shouldn't first overflow to double here

Can we just stick with Java math please? Some of us depend on it.

nickl- · 2024-08-10T11:10:01Z

src/main/java/bsh/Operators.java

+            case POWER:
+            case POWERX:
+                double check = Math.pow(lhs, rhs);
+                if ( Double.isInfinite(check) )


Should we not try and return a float here?

Math.pow returns a double, so no one is expecting to get a float here.

nickl- · 2024-08-10T11:21:19Z

src/main/java/bsh/Operators.java

-                lhs = Long.valueOf(lnum.longValue());
-            if ( !(rhs instanceof Long) )
-                rhs = Long.valueOf(rnum.longValue());
+        if (!(lhs instanceof Float && rhs instanceof Float)) {


It is hard to see what happens here but this seems wrong.

From the existing function:

beanshell/src/main/java/bsh/Operators.java

Lines 513 to 541 in 53b0436

static Object[] promotePrimitives(Object lhs, Object rhs)

{

Number lnum = promoteToInteger(lhs);

Number rnum = promoteToInteger(rhs);

if ( lhs instanceof BigDecimal ) {

if ( !(rhs instanceof BigDecimal) )

rhs = Primitive.castNumber(BigDecimal.class, rnum);

} else if ( rhs instanceof BigDecimal ) {

lhs = Primitive.castNumber(BigDecimal.class, lnum);

} else if ( Types.isFloatingpoint(lhs) || Types.isFloatingpoint(rhs)) {

if ( !(lhs instanceof Double) )

lhs = Double.valueOf(lnum.doubleValue());

if ( !(rhs instanceof Double) )

rhs = Double.valueOf(rnum.doubleValue());

} else if ( lhs instanceof BigInteger ) {

if ( !(rhs instanceof BigInteger) )

rhs = Primitive.castNumber(BigInteger.class, rnum);

} else if ( rhs instanceof BigInteger ) {

lhs = Primitive.castNumber(BigInteger.class, lnum);

} else {

if ( !(lhs instanceof Long) )

lhs = Long.valueOf(lnum.longValue());

if ( !(rhs instanceof Long) )

rhs = Long.valueOf(rnum.longValue());

}

return new Object[] { lhs, rhs };

}

There is a precedence BigDecimal, Double, BigInteger, Long and it seems this check needs to be around line 524 when we are checking for Doubles.

I can refactor this but the outcome will be the same.

nickl- · 2024-08-10T11:27:59Z

src/main/java/bsh/Primitive.java

            throw new InterpreterError("Can only shrink wrap Number types");
        Number value = (Number) number;
+        if (value instanceof Float)
+            return new Primitive(value.floatValue());


I think this needs to do something similar to int and long, comparing the values instead of the type.

See full function 193 and 195:

beanshell/src/main/java/bsh/Primitive.java

Lines 178 to 198 in 53b0436

private static final BigInteger INTEGER_MAX = BigInteger.valueOf(Integer.MAX_VALUE);

private static final BigInteger INTEGER_MIN = BigInteger.valueOf(Integer.MIN_VALUE);

static final BigInteger LONG_MAX = BigInteger.valueOf(Long.MAX_VALUE);

static final BigInteger LONG_MIN = BigInteger.valueOf(Long.MIN_VALUE);

public static Primitive shrinkWrap(Object number) {

if (!(number instanceof Number))

throw new InterpreterError("Can only shrink wrap Number types");

Number value = (Number) number;

if ( Types.isFloatingpoint(number) ) {

if ( !Double.isInfinite(value.doubleValue()) )

return new Primitive(value.doubleValue());

return new Primitive((BigDecimal) number);

}

BigInteger bi = number instanceof BigInteger

? (BigInteger) number : BigInteger.valueOf(value.longValue());

if ( bi.compareTo(INTEGER_MIN) >= 0 && bi.compareTo(INTEGER_MAX) <= 0 )

return new Primitive(bi.intValue());

if ( bi.compareTo(LONG_MIN) >= 0 && bi.compareTo(LONG_MAX) <= 0 )

return new Primitive(bi.longValue());

return new Primitive(bi);

}

I wonder if this would not be sufficient, to just shrink the double result to float if it fits?

If you do the widen float to double, do the operation, and then narrow you will get a different result than doing the operation in float. Best to keep this the same as Java.

In Java byte and short use int as a carrier type. Arithmetic on any combination of these types will yield an int. Best to keep this the same as Java.

opeongo added 2 commits July 29, 2024 15:00

Fix problem of unnecessarily widening float values

adc140a

Fix petty issue that scrutinizer complained about

5d21105

nickl- requested changes Aug 10, 2024

View reviewed changes

Refactor to make the code path more obvious

1f1fe59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix problem of unnecessarily widening float values#768

Fix problem of unnecessarily widening float values#768
opeongo wants to merge 3 commits intomasterfrom
Issue_767

opeongo commented Jul 29, 2024

Uh oh!

nickl- left a comment •

edited

Loading

Uh oh!

nickl- Aug 10, 2024

Uh oh!

opeongo Aug 10, 2024

Uh oh!

nickl- Aug 10, 2024

Uh oh!

opeongo Aug 10, 2024

Uh oh!

nickl- Aug 10, 2024

Uh oh!

opeongo Aug 10, 2024

Uh oh!

nickl- Aug 10, 2024

Uh oh!

opeongo Aug 10, 2024

Uh oh!

nickl- Aug 10, 2024

Uh oh!

opeongo Aug 10, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	static Object[] promotePrimitives(Object lhs, Object rhs)
	{
	Number lnum = promoteToInteger(lhs);
	Number rnum = promoteToInteger(rhs);

	if ( lhs instanceof BigDecimal ) {
	if ( !(rhs instanceof BigDecimal) )
	rhs = Primitive.castNumber(BigDecimal.class, rnum);
	} else if ( rhs instanceof BigDecimal ) {
	lhs = Primitive.castNumber(BigDecimal.class, lnum);
	} else if ( Types.isFloatingpoint(lhs) \|\| Types.isFloatingpoint(rhs)) {
	if ( !(lhs instanceof Double) )
	lhs = Double.valueOf(lnum.doubleValue());
	if ( !(rhs instanceof Double) )
	rhs = Double.valueOf(rnum.doubleValue());
	} else if ( lhs instanceof BigInteger ) {
	if ( !(rhs instanceof BigInteger) )
	rhs = Primitive.castNumber(BigInteger.class, rnum);
	} else if ( rhs instanceof BigInteger ) {
	lhs = Primitive.castNumber(BigInteger.class, lnum);
	} else {
	if ( !(lhs instanceof Long) )
	lhs = Long.valueOf(lnum.longValue());
	if ( !(rhs instanceof Long) )
	rhs = Long.valueOf(rnum.longValue());
	}

	return new Object[] { lhs, rhs };
	}

	private static final BigInteger INTEGER_MAX = BigInteger.valueOf(Integer.MAX_VALUE);
	private static final BigInteger INTEGER_MIN = BigInteger.valueOf(Integer.MIN_VALUE);
	static final BigInteger LONG_MAX = BigInteger.valueOf(Long.MAX_VALUE);
	static final BigInteger LONG_MIN = BigInteger.valueOf(Long.MIN_VALUE);
	public static Primitive shrinkWrap(Object number) {
	if (!(number instanceof Number))
	throw new InterpreterError("Can only shrink wrap Number types");
	Number value = (Number) number;
	if ( Types.isFloatingpoint(number) ) {
	if ( !Double.isInfinite(value.doubleValue()) )
	return new Primitive(value.doubleValue());
	return new Primitive((BigDecimal) number);
	}
	BigInteger bi = number instanceof BigInteger
	? (BigInteger) number : BigInteger.valueOf(value.longValue());
	if ( bi.compareTo(INTEGER_MIN) >= 0 && bi.compareTo(INTEGER_MAX) <= 0 )
	return new Primitive(bi.intValue());
	if ( bi.compareTo(LONG_MIN) >= 0 && bi.compareTo(LONG_MAX) <= 0 )
	return new Primitive(bi.longValue());
	return new Primitive(bi);
	}

Conversation

opeongo commented Jul 29, 2024

Uh oh!

nickl- left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

nickl- left a comment •

edited

Loading