widening bug in IINC (was RE: cvs commit: jakarta-bcel/src/java/org/apache/bcel/generic RET.java)

> Maybe the the fixes contained in bugs 26532 and 26533 could also be 
> committed?  They are extremely trivial fixes for fairly seroius bugs in 
> core BCEL classes (InstructionList and IINC), so I would think they 
> should get some attention.

I agree, this is a bug.  And the suggested fix looks correct to me.

However, here is a slightly improved patch:

--- jakarta-bcel/src/java/org/apache/bcel/generic/IINC.java.~1.3.~
2003-05-23 03:55:17.000000000 -0400
+++ jakarta-bcel/src/java/org/apache/bcel/generic/IINC.java	2004-02-22
21:47:58.000000000 -0500
@@ -106,8 +106,8 @@
   }
 
   private final void setWide() {
-    if(wide = ((n > org.apache.bcel.Constants.MAX_SHORT) ||
-	       (Math.abs(c) > Byte.MAX_VALUE)))
+    if(wide = ((n > Byte.MAX_VALUE) ||
+               (c < Byte.MIN_VALUE) || (c > Byte.MAX_VALUE)))
       length = 6; // wide byte included  
     else
       length = 3;

The current logic is clearly broken:

 wide = (n > MAX_UNSIGNED_SHORT) || (abs(c) > MAX_BYTE)

Andrew's patch changes that (correctly) to:

 wide = (n > MAX_BYTE) || (abs(c) > MAX_BYTE)

but the optimal logic is this:

 wide = (n > MAX_BYTE) || (c < MIN_BYTE) || (c > MAX_BYTE)

-Ed

PS testing just to make sure ... javac on this:

 public class tmp { int foo (int x) { return x += -128; } }

produces this:

 0:   iinc    1, -128
 3:   iload_1
 4:   ireturn