Re: FYI: Patch: java.nio: Reimplementing byte buffer getType/putTypemethods

[Eric Blake <ebb9 at byu dot net>]

Looks better this time around - the only correctness issue I saw this time was 
my bug.  Also in my review, I noticed a couple more optimizations.

Michael Koch wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Very much thanks for you review. I have commited the attached patch to 
> trunk. I hope I have implemented all your suggestions. If you like 
> you can take a look at it again.
>
>
> Michael
> - -- 
> @@ -88,13 +88,13 @@
>  
>      if (buffer.order() == ByteOrder.LITTLE_ENDIAN)
>        {
> -        buffer.put ((byte) (((int) value) & 0x00ff));
> -        buffer.put ((byte) ((((int) value) & 0xff00) >> 8));
> +        buffer.put ((byte) (value & 0x00ff));
> +        buffer.put ((byte) ((value & 0xff00) >> 8));

You can simplify this further, and let the conversion to byte do the masking:
buffer.put ((byte) value);
buffer.put ((byte) (value >> 8));

This will affect quite a few places in the code.

> @@ -106,8 +106,8 @@
>      
>      if (buffer.order() == ByteOrder.LITTLE_ENDIAN)
>        {
> -        return (char) (((buffer.get (index + 1) & 0xff) << 8)
> -                       + (buffer.get (index) & 0xff));
> +        return (char) ((buffer.get (index) & 0xff)
> +                       + ((buffer.get (index + 1) & 0xff) << 8));

Likewise, in building larger types, you can avoid the mask on the most 
significant byte (but the mask is necessary on all other bytes: since byte is 
signed, not masking could corrupt the higher bytes):

return (char) ((buffer.get (index) & 0xff)
               + (buffer.get (index + 1) << 8));


> public static final int getInt (ByteBuffer buffer)

The cast to int here is redundant, but doesn't affect .class or .o files in size.

> @@ -263,10 +263,10 @@
>  
>      if (buffer.order() == ByteOrder.LITTLE_ENDIAN)
>        {
> -        buffer.put (index + 3, (byte) ((value & 0xff000000) >> 24));
> -        buffer.put (index + 2, (byte) ((value & 0x00ff0000) >> 16));
> -        buffer.put (index + 1, (byte) ((value & 0x0000ff00) >> 8));
>          buffer.put (index, (byte) (value & 0x000000ff));
> +        buffer.put (index + 1, (byte) ((value & 0x0000ff00) >> 8));
> +        buffer.put (index + 2, (byte) ((value & 0x00ff0000) >> 16));
> +        buffer.put (index + 3, (byte) ((value & 0xff000000) >> 24));

Another .class file efficiency tip - incrementing local variables uses a 
dedicated bytecode:

buffer.put (++index, (byte) (value >> 8));
buffer.put (++index, (byte) (value >> 16));
buffer.put (++index, (byte) (value >> 24));

> @@ -289,16 +289,16 @@
>                         + ((buffer.get() & 0xff) << 8)
>                         + ((buffer.get() & 0xff) << 16)
>                         + ((buffer.get() & 0xff) << 24)
> -                       + ((buffer.get() & 0xff) << 32)
> -                       + ((buffer.get() & 0xff) << 40)
> -                       + ((buffer.get() & 0xff) << 48)
> -                       + ((buffer.get() & 0xff) << 56));
> +                       + ((buffer.get() & 0xffL) << 32)
> +                       + ((buffer.get() & 0xffL) << 40)
> +                       + ((buffer.get() & 0xffL) << 48)
> +                       + ((buffer.get() & 0xffL) << 56));

My bad - if the fourth get() was negative, the int math will sign-extend and 
ruin the long.  Now that I think about it, you can do even less 64-bit math, 
and this time get the right result:

return ((buffer.get() & 0xff)             // int math until...
        + ((buffer.get() & 0xff) << 8)
        + ((buffer.get() & 0xff) << 16)
        + ((buffer.get() & 0xffL) << 24)  // must be long mask to 0-extend
        + (long) ((buffer.get() & 0xff)   // more int math...
                  + ((buffer.get() & 0xff) << 8)
                  + ((buffer.get() & 0xff) << 16)
                  + (buffer.get() << 24))
           << 32);                         // ...until last minute

Unfortunately, it makes the code less legible, so I won't be offended if you 
can come up with a nicer solution that uses the same (or fewer) shifts and masks.

>    public static final ByteBuffer putFloat (ByteBuffer buffer, float value)
>    {
> +    return putInt (buffer, Float.floatToIntBits (value));
>    }

We should probably use Float.floatToRawIntBits() here, to preserve the hidden 
bits in a NaN.  But I like your redirection to putInt (now why didn't I think 
of that this morning?).

--
Someday, I might put a cute statement here.

Eric Blake             ebb9@byu.net