code review 173670044: x/net/{internal/icmp,ipv4,ipv6}: better method for icmp...

x/net/{internal/icmp,ipv4,ipv6}: better method for icmp.Type interface

[mikio, 2014-11-28 00:33:08]

Hello iant@golang.org (cc: golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go.net

[bradfitz, 2014-11-28 00:37:11]

https://codereview.appspot.com/173670044/diff/60001/ipv6/icmp.go
File ipv6/icmp.go (right):

https://codereview.appspot.com/173670044/diff/60001/ipv6/icmp.go#newcode24
ipv6/icmp.go:24: // Protocol returns the ICMP for IPv6 protocol number.
This sentence doesn't make sense.

It reads like: "Protocol returns the protocol for IPv6 Protocol number".  But
then it returns an integer, which isn't a protocol.

I haven't been following these CLs (which makes me a perfect reviewer for
documentation, since I don't come with background knowledge), but is this trying
to say:

   "Protocol returns the ICMP-over-IPv6 protocol number."

?

[mikio, 2014-11-28 00:42:55]

ptal

https://codereview.appspot.com/173670044/diff/60001/ipv6/icmp.go
File ipv6/icmp.go (right):

https://codereview.appspot.com/173670044/diff/60001/ipv6/icmp.go#newcode24
ipv6/icmp.go:24: // Protocol returns the ICMP for IPv6 protocol number.
On 2014/11/28 00:37:11, bradfitz wrote:
> I haven't been following these CLs (which makes me a perfect reviewer for
> documentation, since I don't come with background knowledge), but is this
trying
> to say:
> 
>    "Protocol returns the ICMP-over-IPv6 protocol number."

how about "returns the protocol number of ICMP for IPv6" ?

[bradfitz, 2014-11-28 00:45:46]

Actually "ICMPv6" is a popular term. Let's use that. "returns the ICMPv6
protocol number" ?


On Thu, Nov 27, 2014 at 4:42 PM, <mikioh.mikioh@gmail.com> wrote:

> ptal
>
>
> https://codereview.appspot.com/173670044/diff/60001/ipv6/icmp.go
> File ipv6/icmp.go (right):
>
> https://codereview.appspot.com/173670044/diff/60001/ipv6/icmp.go#newcode24
> ipv6/icmp.go:24: // Protocol returns the ICMP for IPv6 protocol number.
> On 2014/11/28 00:37:11, bradfitz wrote:
>
>> I haven't been following these CLs (which makes me a perfect reviewer
>>
> for
>
>> documentation, since I don't come with background knowledge), but is
>>
> this trying
>
>> to say:
>>
>
>      "Protocol returns the ICMP-over-IPv6 protocol number."
>>
>
> how about "returns the protocol number of ICMP for IPv6" ?
>
> https://codereview.appspot.com/173670044/
>

[mikio, 2014-11-28 00:52:33]

yup

On Fri, Nov 28, 2014 at 9:45 AM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> Actually "ICMPv6" is a popular term. Let's use that. "returns the ICMPv6
> protocol number" ?
>
>
> On Thu, Nov 27, 2014 at 4:42 PM, <mikioh.mikioh@gmail.com> wrote:
>>
>> ptal
>>
>>
>> https://codereview.appspot.com/173670044/diff/60001/ipv6/icmp.go
>> File ipv6/icmp.go (right):
>>
>> https://codereview.appspot.com/173670044/diff/60001/ipv6/icmp.go#newcode24
>> ipv6/icmp.go:24: // Protocol returns the ICMP for IPv6 protocol number.
>> On 2014/11/28 00:37:11, bradfitz wrote:
>>>
>>> I haven't been following these CLs (which makes me a perfect reviewer
>>
>> for
>>>
>>> documentation, since I don't come with background knowledge), but is
>>
>> this trying
>>>
>>> to say:
>>
>>
>>>     "Protocol returns the ICMP-over-IPv6 protocol number."
>>
>>
>> how about "returns the protocol number of ICMP for IPv6" ?
>>
>> https://codereview.appspot.com/173670044/
>
>

[mikio, 2014-11-28 00:53:11]

Hello iant@golang.org, bradfitz@golang.org (cc:
golang-codereviews@googlegroups.com),

Please take another look.

[iant, 2014-11-28 15:59:10]

LGTM

[mikio, 2014-11-29 01:11:19]

*** Submitted as
https://code.google.com/p/go/source/detail?r=896befd28fdd&repo=net ***

x/net/{internal/icmp,ipv4,ipv6}: better method for icmp.Type interface

LGTM=iant
R=iant, bradfitz
CC=golang-codereviews
https://codereview.appspot.com/173670044