Hi Vyom!
1)
src/java.base/share/classes/sun/net/ext/ExtendedSocketOptions.java
Thank you for fixing ExtendingSocketOption.options0().
It may be better to make the returned set unmodifiable, and then
Collectors.toUnmodifiableSet could be used for convenience:
return options.stream()
.filter((option) -> !option.name().startsWith("TCP_"))
.collect(Collectors.toUnmodifiableSet());
Also, I think Alan suggested to filter out UDP_* options for SOCK_STREAM
here.
2)
src/java.base/unix/classes/java/net/PlainDatagramSocketImpl.java
If you static-import ExtendedSocketOptions.SOCK_DGRAM as in other files,
then the line 80 wouldn't become too long.
The same applies to src/java.base/unix/classes/java/net/PlainSocketImpl.java
3)
src/jdk.net/share/classes/jdk/net/ExtendedSocketOptions.java
Was it intentional that when flowSupported == true, quickAckSupported ==
false, keepAliveOptSupported == true, the TCP_KEEP* options are *not*
added to the resulting set?
If not, then can this set population be organized in more linear way:
Just create an ArrayList, conditionally fill it in and return
unmodifiable set with Set.of(list.toArray()).
Nit: please place the equal sign at line 172 consistently with the
other two inits above.
With kind regards,
Ivan
On 5/11/18 7:43 PM, vyom tewari wrote:
thanks all for review, please find the latest
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.3/index.html).
I address most of the review comments.
Vyom
On Saturday 12 May 2018 12:01 AM, Chris Hegarty wrote:
On 11 May 2018, at 01:04, Alan Bateman <alan.bate...@oracle.com> wrote:
On 10/05/2018 16:21, vyom tewari wrote:
Hi,
Please find the latest
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.2/index.html)
...
It would be better if the channel implementation didn't static
import ExtendedSocketOptions.getInstance as that is a very generic
method method name. As I mentioned previously, you could simplify
all these usages if you add the following to
sun.net.ext.ExtendedSocketOption
static Set<SocketOption<?>> options(int type) { return
getInstance().options(type)); }
+1
A minor comment on tests is that they can use List.of instead of
Arrays.asList.
+1
Otherwise, this looks very good.
-Chris.
P.S. A separate issue, but when reviewing this it reminded me that we
should deprecate-for-removal jdk/net/Sockets.java. It’s functionality
is already supported by a standard API.
--
With kind regards,
Ivan Gerasimov
