Re: socket timeout patch

[Tom Tromey <tromey at redhat dot com>]

>>>>> "Nic" == Nic Ferrier <nferrier@tapsellferrier.co.uk> writes:

Hi Nic.  Thanks for getting back to this.  I think this is an
important patch, and I'd like to get it in as soon as possible.

Nic> The attached file contains some formatting changes over the
Nic> original... I know the policy is not to accept such changes. Can
Nic> someone confirm if the changes enclosed are acceptable, if not I
Nic> would like to provide 2 patches, the first 2 correct the coding
Nic> style of the existing files and the second to provide the fix.

Generally our policy is that big reformatting is ok but must be done
as a separate patch.

Note that parts of our coding standard aren't very well defined.  For
instance, from your patch:

    -java::net::PlainSocketImpl::listen (jint backlog)
    +java::net::PlainSocketImpl::listen(jint backlog)

I think by our current rules this is "incorrect".  My understanding
is that there is no space when making a method call, but that the
space is required in other situations, such as declarations or plain
function calls.  At least, that's how I've tried to do it -- looking
through the code I can see there is some difference of opinion here.
Sometimes, particular in Java code, there isn't a space before `(' in
method declarations.

In other situations, for instance after keywords, the space is
definitely required.  Also it is required around `=':

    +  private InputStream in=null;

should be

    + private InputStream in = null;

By and large I think the existing formatting of these files is ok.
They look "conforming enough" to me.

Nic> I realise that I have to write a changelog as well, I'm
Nic> submitting just the patch at this stage to get confirmation of
Nic> the above issue.

I have a question about the addition of `#include <stdio.h>' into
natPlainSocketImpl.cc.  What is that for?

Also, in PlainSocketImpl::setOption, I see this:

    +  else  // Assume value is an Integer.

Should we check and throw an exception here if this assumption doesn't
hold?  (I realize you didn't write this, but I thought you might know
the answer.)

Tom