RFR 8241305: Add protocol specific factory creation methods to SocketChannel and ServerSocketChannel

[Michael McMahon]

Thanks for the comments again. Just a couple of questions below.

On 15/05/2020 20:48, Alan Bateman wrote:
>
>
> On 15/05/2020 12:46, Michael McMahon wrote:
>> :
>>
>> http://cr.openjdk.java.net/~michaelm/8241305/webrev.5/
> Looks much better.
>
> LocalSocketAddressType.java
>
> Looks okay, just residuals of preferIPv6 in two places, I assume 
> debugging from a previous iteration.
>
>
> ProtocolFamilies.java
>
> What is the reason for the "UOE" rows in the openBind data provider? 
> If I read it correctly, it means the xxOpenBind tests duplicate the 
> xxOpen tests for the UOE case. If the UOE rows can be dropped then it 
> would simplify the xxOpenBind tests and avoid the outer try-catch (I 
> assume the outer try-catch is in case UOE is not thrown but I think it 
> can all go away).
>
I don't follow this. The reason for the UOE rows are to test for UOE 
being thrown when IPv6 is not
available. Are you saying not to test this scenario?

> Minor nit but openBind uses "(hasIPv6 && !preferIPv4)" which is a bit 
> clearer (in my view) that the inverse in the "open" data provider.
>
>
> OpenAndConnect.java
>
> The xxOpenAndConnect tests are much clearer in this iteration. Minor 
> nit is that scOpenAndConnect looks like it print ssa twice, once as a 
> SocketAddress, then its components.
>
> e they needed? I assume openConnect just need to tests that use IPv4 
> local addresses when IA4LOCAL != null.
>
I'm assuming the truncated word refers to previous sentence.


> I think openConnectGen, openConnectV4Local, and openConnectV6Local 
> would benefit from better names as they generate test cases and don't 
> open anything. has_xxlocal also have unusual names, are they needed? I 
> assume the openConnect data provider can use IA4LOCAL != null and 
> IP6LOCAL != null.
>
That's fine. How about adding the suffix "Tests" to the names, eg 
openConnectV4LocalTests?
> Another nit is that this test uses isNotLoopback, I thought the 
> filters using !isLoopback(nif) in the other test was a bit clear. Also 
> would be good to move it above or below isUp as they both test the 
> network interface.
>
I'll change it to use the same initialization code as the other test. It 
doesn't need to handle NetworkInterface
directly and the isUp() method wasn't used anyway.

I won't update the webrev until I hear back from you.

Thanks,

Michael.