On Saturday 12 May 2018 04:14 PM, Ivan Gerasimov wrote:
Thanks Vyom!
I like it much better now.
The last minorish comment:
src/jdk.net/share/classes/jdk/net/ExtendedSocketOptions.java
Set<SocketOption<?>> options = new HashSet<>(5);
Please note that the constructor of HashSet wants the initial capacity
as the argument, not the expected number of elements.
So in this case it would be more accurate to have (7), so that 7 *
0.75 = 5.25 > 5.
Practically, it wouldn't make any difference, as both 5 and 7 would be
rounded up to 8 anyways.
thanks for detail explanation :) , i did not saw the code but i was
under impression that it will use size as the nearest prime number for
uniform distribution of elements in general.
However, I would recommend using the default constructor just to avoid
confusion.
Or, alternatively, collect the options to an ArrayList of desired
capacity and then make unmodifiable set with Set.copyOf(list).
No further comments from my side :)
if no further comment from others as well, i will change it to use
default constructor at time of pushing.
Thanks,
Vyom
With kind regards,
Ivan
On 5/12/18 2:21 AM, vyom tewari wrote:
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.
