code review 4873049: asn1: add support for unmarshalling structs with int32 ...

asn1: add support for unmarshalling structs with int32 members

Also improve error message for unsupported integer types

[dave_cheney.net, 2011-08-14 01:51:13]

Hello agl@chromium.org (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[dave_cheney.net, 2011-08-14 01:58:30]

Hello agl@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[agl1, 2011-08-14 04:05:43]

I won't have a real computer till Tuesday to take a look at this, but my
thought is: what happens on overflow in parsing? It might be that we don't
care and nor does the current code but I'm just writing a reminder to check.
On Aug 13, 2011 6:58 PM, <dave@cheney.net> wrote:
> Hello agl@golang.org (cc: golang-dev@googlegroups.com),
>
> Please take another look.
>
>
> http://codereview.appspot.com/4873049/

[dave_cheney.net, 2011-08-14 04:12:38]

Wrt int32 parsing, I'm just wrapping the existing ParseInt method, so truncation
remains unchanged when unmarshalling. 

The impetus for this CL came from a frustrating weekend lost, which was totally
my fault, trying to parse SNMP PDUs when I transliterated the definition of the
requestID field as int32 not int. 

If there isn't much interest in expanding the types that Unmarshall can handle,
i'd still prefer I see the expanded error handling go in. 

Cheers

Dave

Sent from my iPhone

On 14/08/2011, at 14:05, Adam Langley <agl@golang.org> wrote:

> I won't have a real computer till Tuesday to take a look at this, but my
thought is: what happens on overflow in parsing? It might be that we don't care
and nor does the current code but I'm just writing a reminder to check.
> 
> On Aug 13, 2011 6:58 PM, <dave@cheney.net> wrote:
> > Hello agl@golang.org (cc: golang-dev@googlegroups.com),
> > 
> > Please take another look.
> > 
> > 
> > http://codereview.appspot.com/4873049/

[dave_cheney.net, 2011-08-14 05:42:10]

Please take another look. Realising that int and int32 were the same thing, I've
reduced the size of the CL by swapping wrapper methods for a cast.

[agl1, 2011-08-16 18:06:42]

LGTM. (Ping me if you need me to land it.)

http://codereview.appspot.com/4873049/diff/3003/src/pkg/asn1/asn1.go
File src/pkg/asn1/asn1.go (right):

http://codereview.appspot.com/4873049/diff/3003/src/pkg/asn1/asn1.go#newcode701
src/pkg/asn1/asn1.go:701: //TODO(dfc) Add support for the remaining integer
variants
nit: space after the "//"

http://codereview.appspot.com/4873049/diff/3003/src/pkg/asn1/asn1.go#newcode702
src/pkg/asn1/asn1.go:702: err = StructuralError{fmt.Sprintf("unknown integer
type: %s", val.Type().Kind())}
err = StructuralError{"unsupported integer type: " + val.Type().Kind().String}

[rsc, 2011-08-16 20:36:51]

http://codereview.appspot.com/4873049/diff/3003/src/pkg/asn1/asn1.go
File src/pkg/asn1/asn1.go (right):

http://codereview.appspot.com/4873049/diff/3003/src/pkg/asn1/asn1.go#newcode684
src/pkg/asn1/asn1.go:684: case reflect.Int, reflect.Int8, reflect.Int16,
reflect.Int32, reflect.Int64:
This an auto-fixed line.  You can change this to

case reflect.Int, reflect.Int32:
    parseInt, err1 := ...

case reflect.Int64:
    ...

and leave the others out (i.e. pull the inner switch up).

Then let the StructuralError at the bottom handle the 
unknown types.  Feel free to add type information down there.

http://codereview.appspot.com/4873049/diff/3003/src/pkg/asn1/asn1.go#newcode766
src/pkg/asn1/asn1.go:766: err = StructuralError{"unknown Go type"}
err = StructuralError{"unknown type: " + val.Type().String()}

[dave_cheney.net, 2011-08-16 21:51:06]

Thanks for the feedback, I will update and resubmit tonight. 

Sent from my iPad

On 17/08/2011, at 6:36, rsc@golang.org wrote:

> 
> http://codereview.appspot.com/4873049/diff/3003/src/pkg/asn1/asn1.go
> File src/pkg/asn1/asn1.go (right):
> 
>
http://codereview.appspot.com/4873049/diff/3003/src/pkg/asn1/asn1.go#newcode684
> src/pkg/asn1/asn1.go:684: case reflect.Int, reflect.Int8, reflect.Int16,
> reflect.Int32, reflect.Int64:
> This an auto-fixed line.  You can change this to
> 
> case reflect.Int, reflect.Int32:
>    parseInt, err1 := ...
> 
> case reflect.Int64:
>    ...
> 
> and leave the others out (i.e. pull the inner switch up).
> 
> Then let the StructuralError at the bottom handle the
> unknown types.  Feel free to add type information down there.
> 
>
http://codereview.appspot.com/4873049/diff/3003/src/pkg/asn1/asn1.go#newcode766
> src/pkg/asn1/asn1.go:766: err = StructuralError{"unknown Go type"}
> err = StructuralError{"unknown type: " + val.Type().String()}
> 
> http://codereview.appspot.com/4873049/

[dave_cheney.net, 2011-08-17 11:13:19]

Please take another look

http://codereview.appspot.com/4873049/diff/3003/src/pkg/asn1/asn1.go
File src/pkg/asn1/asn1.go (right):

http://codereview.appspot.com/4873049/diff/3003/src/pkg/asn1/asn1.go#newcode684
src/pkg/asn1/asn1.go:684: case reflect.Int, reflect.Int8, reflect.Int16,
reflect.Int32, reflect.Int64:
On 2011/08/16 20:36:55, rsc wrote:
> This an auto-fixed line.  You can change this to
> 
> case reflect.Int, reflect.Int32:
>     parseInt, err1 := ...
> 
> case reflect.Int64:
>     ...
> 
> and leave the others out (i.e. pull the inner switch up).
> 
> Then let the StructuralError at the bottom handle the 
> unknown types.  Feel free to add type information down there.

Done.

http://codereview.appspot.com/4873049/diff/3003/src/pkg/asn1/asn1.go#newcode701
src/pkg/asn1/asn1.go:701: //TODO(dfc) Add support for the remaining integer
variants
On 2011/08/16 18:06:43, agl1 wrote:
> nit: space after the "//"

Done.

http://codereview.appspot.com/4873049/diff/3003/src/pkg/asn1/asn1.go#newcode702
src/pkg/asn1/asn1.go:702: err = StructuralError{fmt.Sprintf("unknown integer
type: %s", val.Type().Kind())}
On 2011/08/16 18:06:43, agl1 wrote:
> err = StructuralError{"unsupported integer type: " + val.Type().Kind().String}

Done.

[rsc, 2011-08-17 15:23:56]

LGTM

[rsc, 2011-08-17 16:12:05]

*** Submitted as http://code.google.com/p/go/source/detail?r=98333fd8a047 ***

asn1: add support for unmarshalling structs with int32 members

Also improve error message for unsupported integer types

R=agl, rsc
CC=golang-dev
http://codereview.appspot.com/4873049

Committer: Russ Cox <rsc@golang.org>