On 27/05/2019 11:48, Chris Hegarty wrote:
This next iteration addresses all of Alan's comments and suggestions.
Additionally, while here we can take the opportunity to cleanup the spec
inconsistencies of the getOption/setOption methods across the socket
impls. The current default implementation is also problematic,
undocumented and unspecified. It is highly questionable for code to
depend on it. The default implementation can be simplified, specified,
and a note advising subclasses to override added. ( The CSR contains
specifics, that I won't duplicate here )
Updated webrev:
https://cr.openjdk.java.net/~chegar/8224477/webrev.03/
CSR:
https://bugs.openjdk.java.net/browse/JDK-8224792
This looks a good cleanup. There may be an argument to have the default
implementations of get/setOption throw NPE when name is null.
Does isServer need to be package-private? I didn't spot any usages in
PSI/PDSI.
For SocketChannelImpl/ServerSocketChannelImp then I'd prefer to use the
same check that we have in the new NioSocketImpl to test that the value
type matches the SocketOption::type. We can do the same check in
DatagramChannelImpl.setOption.
The tests looks good. A suggestion for the TestImplSpec methods is to
rename them to something like TestDefaultBehavior as I think that is
better describes what they test.
-Alan
