On Tue, 16 Feb 2021 13:31:13 GMT, Jamie Le Tual
<github.com+55101029+jamielet...@openjdk.org> wrote:
>> src/java.base/unix/native/libnet/Inet6AddressImpl.c line 713:
>>
>>> 711: This usually requires "root" privileges, so it's likely to fail.
>>> 712: If all else fails, fall back to TCP and implement tcp echo
>>> 713: */
>>
>> This is one of the block comments that needs a tidy, same thing in
>> Inet4Address.c. Also check the // comments and you'll see some of the
>> inconsistencies there. It's just nit picking, the patch itself is good, just
>> hard to test.
>
> OK, is it the fact that it's a block comment instead of a line comment? Is
> the indentation not right?
> I don't see what needs to be cleaned up
I think what Alan said was in reference to the overall styling of the block
comment. For example, something like this with more consistent spacing may be
more appropriate (note the minimal spacing at the end of sentences where
possible). Perhaps the block comment could be indented also to match the block
of code its concerning.
/*
First we see if the OS supports ICMP as a protocol. In the event
that this is an older kernel without IPPROTO_ICMP or that the
net.ipv4.ping_group_range kernel parameter is not set, then we
fall back to trying to create a RAW socket to send ICMP packets.
This usually requires "root" privileges, so it's likely to fail.
If all else fails, fall back to TCP and implement tcp echo
*/
Also, is there any scope for simplifying the comments a bit so that they could
be shortened to 2-3 line comments? For example
// Check OS supports ICMP. If IPPROTO_ICMP is not present or
// the net.ipv4.ping_group_range kernel parameter is not set, fall
// back to trying to create a RAW socket to send ICMP packets.
// If all else fails, fall back to TCP and implement tcp echo.
The second comment is more of a suggestion concerning formatting and not what I
think should be present. Having simple and concise comments is always
beneficial for future maintenance in my opinion.
-------------
PR: https://git.openjdk.java.net/jdk/pull/1502
