Hi Ivan,
Thanks for review, please find the updated
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.4/index.html).
Please find my answers in-line.
Thanks,
vyom
On Saturday 12 May 2018 11:15 AM, Ivan Gerasimov wrote:
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.
fixed
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
fixed
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()).
of course not, i don't know but i always prefer complex nested
"if-else" then linear ifs :) fixed as well.
Nit: please place the equal sign at line 172 consistently with the
other two inits above.
fixed.
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.
