Re: Socket timeout patch

[Bryce McKinlay <bryce at waitaki dot otago dot ac dot nz>]

Nic Ferrier wrote:

>>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).
>

OK, fair enough - that makes sense. But I think that duplication should 
be avoided and they should always be complete sentences. For example:

/** the socket's file handle.
 * This is the socket handle that is returned from the creation
 * of the socket. It is used for read and writes to the socket and
 * to close the socket.
 *

Should be more like:

/**
 * The socket's file handle.
 * This is the value returned by the creation of the socket. It is 
 * used for read and writes to the socket and to close the socket.
 *

>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?
>

It is the convention followed throughout libjava and classpath. It would 
be nice to keep it consistent.

>>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.
>

I agree that it makes the code clearer in some cases to explicitly 
specify it, but it does cause GCJ to generate less efficient code. 
Perhaps we can fix GCJ to remove/ignore the redundant assignment though. 
Do other bytecode compilers generate field initializer code for null and 
zero initializations?

>>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)
>

No. Only the java compiler ever generates class objects - so there is 
only one class object  even if the class has native methods. In this 
case, since the SocketInput/OutputStream classes are inner classes 
anyway, I see no reason to not just implement their native methods in 
natPlainSocketImpl.cc also.

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

Its not the "this" pointer that makes it less efficient but rather the 
extra call. Since they are native methods, GCJ cannot inline them (and 
since the SocketInput/OutputStream methods are public and virtual, they 
are difficult to inline also).

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

It reduces the total number of methods, which reduces code size, 
particularly since every method in Java requires metadata for reflection 
and such to be attached.

>>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 seem to remember seeing "connection reset by peer" exceptions when I 
was last working on a sockets application with gcj. Perhaps I am 
mistaken or gcj was incorrect to throw it, but the code does seem 
inconsistent - it throws for ECONNRESET in the read() case but not in 
the write() case.

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

Just because fnum == 0 is a "normal" condition which indicates close() 
has been called, while an EBADF in other cases would suggest some other 
error condition/bug in our code and it would be nice to throw in that case.

>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.
>

I'm sorry to be so pedantic, but it is important to try to keep the code 
consistent and clean in order to preserve its long-term maintainability.

regards

Bryce.