[RFA] gnu.gcj.jdwp.transport: packet classes

[Keith Seitz]

On Sun, 2005-05-29 at 12:23 -0600, Tom Tromey wrote:
> Keith> +  public int getLength () { return MINIMUM_LENGTH + super.getLength (); }
> 
> Usually we don't write functions all on one line like this.  It
> doesn't matter to me though as this is still perfectly clear and
> readable.  However there are one or two cases where it looks like this
> goes over the 80 character line limit; in that case breaking it up is
> better.

Ah, see now people are learning that I am a Java neophyte. This is a
carry-over from the way I write C++ class setter/getter methods (inlined
in the class decl). I'll change all those (and double-check line length
limits).

> Keith> +    int length = (bytes[i++] << 24) | (bytes[i++] << 16) | (bytes[i++] << 8) | bytes[i++];
> 
> I think this yields the wrong result if any of the bytes[n] has the
> high bit set, due to sign extension.  You need
> '(bytes[i++] & 0xff) << 16' (etc) instead.

Good catch. I missed that one. [I find it difficult to remember that a
byte is a signed quantity!] I've corrected a similar problem in
JdwpReplyPacket.

> Java really needs a utility function for this -- we probably have this
> bug elsewhere in the tree.  I wonder if FindBugs can catch it.
> 
> Also this line needs to be wrapped at 79 chars.
> 
> Keith> +    bytes[i++] = (byte) ((length >>> 24) & 0xff);
> 
> These don't need the '& 0xff' as the cast to byte does that
> implicitly.  But if you want to keep this for extra clarity, I won't
> complain.

Once again, my history with C is responsible for that. You are correct,
it is not necessary, and I will remove it.

> Keith> +    // it won't leave holes int the outgoing packet ids).
> 
> Small typo, s/int/in/

Fixed.

Thanks,
Keith