Hi Vyom, looks good to me.
Best regards Christoph > -----Original Message----- > From: net-dev [mailto:net-dev-boun...@openjdk.java.net] On Behalf Of > vyom tewari > Sent: Donnerstag, 19. Oktober 2017 04:56 > To: net-dev@openjdk.java.net > Subject: Re: RFR 8145635 : Add TCP_QUICKACK socket option > > Hi All, > > please find the latest > webrev(http://cr.openjdk.java.net/~vtewari/8145635/webrev0.7/index.htm > l). > > Thanks, > > Vyom > > > On Wednesday 18 October 2017 08:21 PM, Langer, Christoph wrote: > > 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 > >>>>
