Code review request for 6897550 BigInteger constructor should use local cached String length

[Joe Darcy]

Hello.

Please review this simple change to replace several calls to
val.length() with an already cached copy of val.length(); patch below.

6897550 BigInteger constructor should use local cached String length
http://cr.openjdk.java.net/~darcy/6897550.0/

-Joe

--- old/src/share/classes/java/math/BigInteger.java    2009-11-03
13:57:31.000000000 -0800
+++ new/src/share/classes/java/math/BigInteger.java    2009-11-03
13:57:31.000000000 -0800
@@ -292,7 +292,7 @@
 
         if (radix < Character.MIN_RADIX || radix > Character.MAX_RADIX)
             throw new NumberFormatException("Radix out of range");
-        if (val.length() == 0)
+        if (len == 0)
             throw new NumberFormatException("Zero length BigInteger");
 
         // Check for at most one leading sign
@@ -303,7 +303,7 @@
             // No leading sign character or at most one leading sign
character
             if (index1 == 0 || index2 == 0) {
                 cursor = 1;
-                if (val.length() == 1)
+                if (len == 1)
                     throw new NumberFormatException("Zero length
BigInteger");
             }
             if (index1 == 0)
@@ -342,7 +342,7 @@
         // Process remaining digit groups
         int superRadix = intRadix[radix];
         int groupVal = 0;
-        while (cursor < val.length()) {
+        while (cursor < len) {
             group = val.substring(cursor, cursor += digitsPerInt[radix]);
             groupVal = Integer.parseInt(group, radix);
             if (groupVal < 0)

[gnu_andrew]

2009/11/3 Joseph D. Darcy <Joe.Darcy@...>:

> Hello.
>
> Please review this simple change to replace several calls to val.length()
> with an already cached copy of val.length(); patch below.
>
> 6897550 BigInteger constructor should use local cached String length
> http://cr.openjdk.java.net/~darcy/6897550.0/
>
> -Joe
>
> --- old/src/share/classes/java/math/BigInteger.java    2009-11-03
> 13:57:31.000000000 -0800
> +++ new/src/share/classes/java/math/BigInteger.java    2009-11-03
> 13:57:31.000000000 -0800
> @@ -292,7 +292,7 @@
>
>        if (radix < Character.MIN_RADIX || radix > Character.MAX_RADIX)
>            throw new NumberFormatException("Radix out of range");
> -        if (val.length() == 0)
> +        if (len == 0)
>            throw new NumberFormatException("Zero length BigInteger");
>
>        // Check for at most one leading sign
> @@ -303,7 +303,7 @@
>            // No leading sign character or at most one leading sign
> character
>            if (index1 == 0 || index2 == 0) {
>                cursor = 1;
> -                if (val.length() == 1)
> +                if (len == 1)
>                    throw new NumberFormatException("Zero length
> BigInteger");
>            }
>            if (index1 == 0)
> @@ -342,7 +342,7 @@
>        // Process remaining digit groups
>        int superRadix = intRadix[radix];
>        int groupVal = 0;
> -        while (cursor < val.length()) {
> +        while (cursor < len) {
>            group = val.substring(cursor, cursor += digitsPerInt[radix]);
>            groupVal = Integer.parseInt(group, radix);
>            if (groupVal < 0)
>
>

Looks good to me.  This is all in one method, and len doesn't change
its value (maybe make it final?)
--
Andrew :-)

Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

Support Free Java!
Contribute to GNU Classpath and the OpenJDK
http://www.gnu.org/software/classpath
http://openjdk.java.net

PGP Key: 94EFD9D8 (http://subkeys.pgp.net)
Fingerprint: F8EF F1EA 401E 2E60 15FA  7927 142C 2591 94EF D9D8

[Christopher Hegarty -Sun Microsystems Ireland]

Thanks Joe, looks good.

-Chris.

Joseph D. Darcy wrote:

> Hello.
>
> Please review this simple change to replace several calls to
> val.length() with an already cached copy of val.length(); patch below.
>
> 6897550 BigInteger constructor should use local cached String length
> http://cr.openjdk.java.net/~darcy/6897550.0/
>
> -Joe
>
> --- old/src/share/classes/java/math/BigInteger.java    2009-11-03
> 13:57:31.000000000 -0800
> +++ new/src/share/classes/java/math/BigInteger.java    2009-11-03
> 13:57:31.000000000 -0800
> @@ -292,7 +292,7 @@
>
>         if (radix < Character.MIN_RADIX || radix > Character.MAX_RADIX)
>             throw new NumberFormatException("Radix out of range");
> -        if (val.length() == 0)
> +        if (len == 0)
>             throw new NumberFormatException("Zero length BigInteger");
>
>         // Check for at most one leading sign
> @@ -303,7 +303,7 @@
>             // No leading sign character or at most one leading sign
> character
>             if (index1 == 0 || index2 == 0) {
>                 cursor = 1;
> -                if (val.length() == 1)
> +                if (len == 1)
>                     throw new NumberFormatException("Zero length
> BigInteger");
>             }
>             if (index1 == 0)
> @@ -342,7 +342,7 @@
>         // Process remaining digit groups
>         int superRadix = intRadix[radix];
>         int groupVal = 0;
> -        while (cursor < val.length()) {
> +        while (cursor < len) {
>             group = val.substring(cursor, cursor += digitsPerInt[radix]);
>             groupVal = Integer.parseInt(group, radix);
>             if (groupVal < 0)
>

[Joe Darcy]

Andrew John Hughes wrote:

> 2009/11/3 Joseph D. Darcy <Joe.Darcy@...>:
>  
>> Hello.
>>
>> Please review this simple change to replace several calls to val.length()
>> with an already cached copy of val.length(); patch below.
>>
>> 6897550 BigInteger constructor should use local cached String length
>> http://cr.openjdk.java.net/~darcy/6897550.0/
>>
>> -Joe
>>
>> --- old/src/share/classes/java/math/BigInteger.java    2009-11-03
>> 13:57:31.000000000 -0800
>> +++ new/src/share/classes/java/math/BigInteger.java    2009-11-03
>> 13:57:31.000000000 -0800
>> @@ -292,7 +292,7 @@
>>
>>        if (radix < Character.MIN_RADIX || radix > Character.MAX_RADIX)
>>            throw new NumberFormatException("Radix out of range");
>> -        if (val.length() == 0)
>> +        if (len == 0)
>>            throw new NumberFormatException("Zero length BigInteger");
>>
>>        // Check for at most one leading sign
>> @@ -303,7 +303,7 @@
>>            // No leading sign character or at most one leading sign
>> character
>>            if (index1 == 0 || index2 == 0) {
>>                cursor = 1;
>> -                if (val.length() == 1)
>> +                if (len == 1)
>>                    throw new NumberFormatException("Zero length
>> BigInteger");
>>            }
>>            if (index1 == 0)
>> @@ -342,7 +342,7 @@
>>        // Process remaining digit groups
>>        int superRadix = intRadix[radix];
>>        int groupVal = 0;
>> -        while (cursor < val.length()) {
>> +        while (cursor < len) {
>>            group = val.substring(cursor, cursor += digitsPerInt[radix]);
>>            groupVal = Integer.parseInt(group, radix);
>>            if (groupVal < 0)
>>
>>
>>    
>
> Looks good to me.  This is all in one method, and len doesn't change
> its value (maybe make it final?)
>  

Correct on the nature of len; will add final before I push.

Thanks,

-Joe