code review 86960045: encoding/asn1: only omit optional elements matching def...

encoding/asn1: only omit optional elements matching default value.

ASN.1 elements can be optional, and can have a default value.
Traditionally, Go has omitted elements that are optional and that have
the zero value. I believe that's a bug (see [1]).

This change causes an optional element with a default value to only be
omitted when it has that default value. The previous behaviour of
omitting optional, zero elements with no default is retained because
it's used quite a lot and will break things if changed.

[1] https://groups.google.com/d/msg/Golang-nuts/9Ss6o9CW-Yo/KL_V7hFlyOAJ

Fixes issue 7780.

R=bradfitz

[agl1, 2014-04-14 19:00:02]

Hello golang-codereviews@googlegroups.com,

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

[bradfitz, 2014-04-14 19:56:12]

LGTM but wait for rsc and r since this is late in Go 1.3.

https://codereview.appspot.com/86960045/diff/40001/src/pkg/encoding/asn1/asn1.go
File src/pkg/encoding/asn1/asn1.go (right):

https://codereview.appspot.com/86960045/diff/40001/src/pkg/encoding/asn1/asn1...
src/pkg/encoding/asn1/asn1.go:825: // canHaveDefaultValue returns true iff k is
a Kind that we will set a default
s/returns true iff/reports whether/

https://codereview.appspot.com/86960045/diff/40001/src/pkg/encoding/asn1/mars...
File src/pkg/encoding/asn1/marshal.go (right):

https://codereview.appspot.com/86960045/diff/40001/src/pkg/encoding/asn1/mars...
src/pkg/encoding/asn1/marshal.go:517: defaultValue :=
reflect.New(v.Type()).Elem()
I suppose this works. If you wrote a little function you could avoid this
allocation, though, like:

    func intKind(v int64, k reflect.Kind) interface{} {
          case reflect.Int:
              return int(v)
          case reflect.Int8
              return int8(v)
    ....

And then reflect.DeepEqual(v.Interface(), intKind(*params.DefaultValue,
v.Kind())

Not sure what all uses the ASN marshaller and to what degree we care about
performance here.

[rsc, 2014-04-14 20:00:51]

If you (agl) are sure the new behavior is the right behavior, then it's
fine for Go 1.3, and please add a note to doc/go1.3.html as well.
If you are less sure, then it might be worth leaving as is and waiting for
Go 1.4.

Your call.
Russ

[agl1, 2014-04-16 18:11:25]

On Mon, Apr 14, 2014 at 1:00 PM, Russ Cox <rsc@golang.org> wrote:
> If you (agl) are sure the new behavior is the right behavior, then it's fine
> for Go 1.3, and please add a note to doc/go1.3.html as well.
> If you are less sure, then it might be worth leaving as is and waiting for
> Go 1.4.
>
> Your call.

I'm sure that the current behaviour is wrong.

I don't think the patch is quite right, but it's conservative. If this
landed then I would do a followup in 1.4.

It sounds, on that basis, that it should wait until 1.4.


Cheers

AGL

[rsc, 2014-04-16 18:13:32]

Let's leave it for 1.4 then, and there will only be one API change.

[rsc, 2014-04-17 02:41:32]

R=close

Feel free to pick up after 1.3; just removing from 1.3 dashboard.

[agl1, 2014-07-24 22:50:28]

R=bradfitz

Opening this back up because it's Go 1.4 now. I originally said that I'd want to
tweak this in 1.4 but I'm actually resubmitting as is. I do somewhat regret the
ASN.1 interface in Go now, but I think this CL is the best solution without
breaking things.

[bradfitz, 2014-07-25 18:01:51]

LGTM

Be sure to do a follow-up CL adding a note to go1.4.txt (modifying that file
doesn't require code review).

[agl1, 2014-07-28 21:47:44]

*** Submitted as https://code.google.com/p/go/source/detail?r=4218b3544610 ***

encoding/asn1: only omit optional elements matching default value.

ASN.1 elements can be optional, and can have a default value.
Traditionally, Go has omitted elements that are optional and that have
the zero value. I believe that's a bug (see [1]).

This change causes an optional element with a default value to only be
omitted when it has that default value. The previous behaviour of
omitting optional, zero elements with no default is retained because
it's used quite a lot and will break things if changed.

[1] https://groups.google.com/d/msg/Golang-nuts/9Ss6o9CW-Yo/KL_V7hFlyOAJ

Fixes issue 7780.

R=bradfitz

LGTM=bradfitz
R=golang-codereviews, bradfitz, rsc
CC=golang-codereviews, r
https://codereview.appspot.com/86960045