Hi Vyom,
I just looked at your latest webrev which in general looks fine to me. Some
minor remarks:
make/lib/Lib-jdk.net.gmk:
- copyright year needs to be updated
src/java.base/unix/classes/java/net/PlainDatagramSocketImpl.java:
- private void addExtSocketOptions(...) looks a bit awful concerning its
formatting. I suggest something like this:
private void addExtSocketOptions(Set<SocketOption<?>> extOptions,
Set<SocketOption<?>> options) {
// TCP_QUICKACK is applicable for TCP Sockets only.
extOptions.stream()
.filter((option) -> !option.name().equals("TCP_QUICKACK"))
.forEach((option) -> options.add(option));
}
src/jdk.net/share/classes/jdk/net/ExtendedSocketOptions.java:
- copyright year needs to be updated
- the equals sign should be placed on line 98 here:
98 public static final SocketOption<Boolean> TCP_QUICKACK
99 = new ExtSocketOption<Boolean>("TCP_QUICKACK", Boolean.class);
Other than that you should consider it reviewed from my end. No need for
further webrev...
Best regards
Christoph
> -----Original Message-----
> From: net-dev [mailto:net-dev-boun...@openjdk.java.net] On Behalf Of
> vyom tewari
> Sent: Dienstag, 17. Oktober 2017 10:37
> To: net-dev@openjdk.java.net
> Subject: Re: RFR 8145635 : Add TCP_QUICKACK socket option
>
> Hi Roger,
>
> Thanks for the review , please find the updated
> webrev(http://cr.openjdk.java.net/~vtewari/8145635/webrev0.6/index.htm
> l).
>
> Thanks,
>
> Vyom
>
>
> On Tuesday 17 October 2017 06:35 AM, Roger Riggs wrote:
> > Hi Vyom,
> >
> > A few suggestions:
> >
> > PlainDatagramSocketImpl.java:
> > - line 95/96: I think you can use just forEach, the order version is
> > not necessary.
> > The code will be a bit more readable if the .filter and .forEach
> > are on a new line and don't wrap.
> > You can also remove the extra "(" and ")
> >
> > - line 87-94: these are confusing and imply some implicit resetting
> > of the option.
> > - use @since 10
> > - 209/268: the native setQuickAck method should use boolean as its
> > argument to enable/disable
> > Since enable is a boolean; it does not need "== true'
> >
> > LinuxSocketOptions.java/c:
> > - 52: setQuickAck0 should use boolean for the 2nd argument; (The
> > native code already does)
> >
> > Thanks, Roger
> >
> >
> > On 10/15/17 11:58 PM, vyom tewari wrote:
> >> Hi Chris,
> >>
> >> Thanks for review. Please find the latest
> >>
> webrev(http://cr.openjdk.java.net/~vtewari/8145635/webrev0.5/index.htm
> l).
> >>
> >> Thanks,
> >>
> >> Vyom
> >>
> >
