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

[Tom Tromey <tromey at redhat dot com>]

>>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes:

Keith> [Yes, I do have unit tests for this. Someone will send testsuite patches
Keith> soon.]

Awesome.

Keith> Questions/comments/concerns?

Just a few nits to pick.  Nothing serious.

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.

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.

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.

Keith> +    // it won't leave holes int the outgoing packet ids).

Small typo, s/int/in/

Tom