Re: Socket timeout patch

[Nic Ferrier <nferrier at tf1 dot tapsellferrier dot co dot uk>]

>Actually I have found a few nits to pick ;-) 

Oh for God's sake.


>>
>>-
>> class PlainSocketImpl extends SocketImpl
>> {
>>   // These fields are mirrored for use in native code to avoid cpp conflicts
>>@@ -35,6 +35,18 @@
>>                    _Jv_SO_SNDBUF_ = SocketOptions.SO_SNDBUF,
>>                    _Jv_SO_RCVBUF_ = SocketOptions.SO_RCVBUF;
>> 
>>+  /** the socket's file handle.
>>+   * This is the socket handle that is returned from the creation
>>

>First line of comment is redundant, and the Javadoc comments should 
>always start on the second line (there are several places where this 
>should be fixed in your patch).

I don't agree with that.

And GCJ's javadoc is not very good right now. The Sun Javadoc
standards say that the first line of the javadoc should be a very
quick description of what is being documented, if more is needed then
it is provided on subsequent lines (and this convention fits in nicely
with most info conventions).

AFAIK there is no standard for having doc comments start on the second
line. I have always felt that the summary line should be on the first
line because it makes things look nice.

Is this part of the GCJ code standard? Does it have to change before
the patch is accepted?


>>
>>+  // Stream handling.
>>+
>>+  /** a cached copy of the in stream.
>>+   */
>>+  private InputStream in=null;
>>+
>>+  /** a cached copy of the out stream.
>>+   */
>>+  private OutputStream out=null;
>>
>Javadocs need to start on second line and start with capital letter. 

Harumph.


>Also, the in=null is formatted wrong (should be "in = null"), 

Accepted.


>but in this case the assignments are redundant and should be removed 
>(initializing to zero or null is not neccessary in Java, and it causes 
>GCJ to generate redundant finit code).

Maybe, but it also makes clear what is going on. The variables are
cache variables, Someone reading the related methods might not
understand them.


>>
>>+  /** input stream which reads from the socket implementation.
>>+   *
>>+   * @author Nic Ferrier, nferrier@tapsellferrier.co.uk
>>+   */
>>+  class SocketInputStream
>>+    extends InputStream
>>+  {
>>+    SocketInputStream()
>>+    {
>>+    }
>>+    
>>+    public void close() throws IOException
>>+    {
>>+      PlainSocketImpl.this.close();
>>+    }
>>+
>>+    public int available() throws IOException
>>+    {
>>+      return PlainSocketImpl.this.available();
>>+    }
>>
>Why were these implemented by delegating back to private natives in the 
>SocketImpl rather than making them native themselves? It would be more 
>efficient and reduce the method count.

If they were themselves natives, wouldn't that mean an extra 2 classes?
(with associated C++ equivalents)

Doesn't GCJ just optimize away the inner this pointer?

Why would doing it differently reduce the method count? Do you mean
the method stack?


>>+  while (r != 1)
>>+    {
>>+      r = ::write (fnum, &d, 1);
>>
>Won't this loop forever in the case of an error that doesn't cause 
>IOException?

Maybe... I don't think I changed it from where I got it (the natFile
stuff). Certainly, I didn't see that during testing, but it looks like
you're right.


>>
>>+	  // Some errors should not cause exceptions.
>>+	  if (errno != ENOTCONN && errno != ECONNRESET && errno != EBADF)
>>+	    throw new java::io::IOException (JvNewStringUTF (strerror (errno)));
>>

>Hmm, are you sure that ECONNRESET shouldn't throw? Also maybe it would 
>be better to check for fnum
>== 0 explicitly for the EBADF case?

Well, now I can't remember because it's been so long since I actually
did the work for this patch.

I did some fairly extensive testing at the time, I'm pretty sure it's
right.

I don't think the test for fnum is better either... but I can't really
remember now.


>>+  else if (r == -1)
>>+    {
>>+      if (errno == ECONNRESET)
>>+	throw new java::io::IOException (JvNewStringUTF (strerror (errno)));
>>+      // Some errors cause us to return end of stream...
>>+      if (errno == ENOTCONN)
>>+	return -1;
>>+      // Other errors need to be signalled.
>>+      throw new java::io::IOException (JvNewStringUTF (strerror (errno)));
>>
>Redundant ECONNRESET check?

Don't think so. If it's there it's because it has to be.


Sorry if some of these responses are a bit *short*. I feel like I've
been through the mill on this change a bit.

It seems sometimes that code standards are more important than getting
something that actually works: this is an important patch, lots of
popular java stuff won't work without it (lots of http stuff for
example).

Part of the reason it's been held up has been my fault, but a lot has
been the fault of the GCJ project.


Nic