code review 5639054: encoding/binary: hide TotalSize

encoding/binary: hide TotalSize

The function has a bizarre signature: it was the only public function there
that exposed the reflect package. Also, its definition is peculiar and hard to
explain. It doesn't merit being exported.

This is an API change but really, it should never have been exported and
it's certain very few programs will depend on it: it's too weird.

Fixes issue 2846.

[r, 2012-02-08 02:13:12]

Hello golang-dev@googlegroups.com,

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

[gri, 2012-02-08 02:16:34]

LGTM
- gri

On Tue, Feb 7, 2012 at 6:13 PM,  <r@golang.org> wrote:
> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> encoding/binary: hide TotalSize
>
> The function has a bizarre signature: it was the only public function
> there
> that exposed the reflect package. Also, its definition is peculiar and
> hard to
> explain. It doesn't merit being exported.
>
> This is an API change but really, it should never have been exported and
> it's certain very few programs will depend on it: it's too weird.
>
> Fixes issue 2846.
>
> Please review this at http://codereview.appspot.com/5639054/
>
> Affected files:
>  M doc/go1.html
>  M doc/go1.tmpl
>  M src/pkg/encoding/binary/binary.go
>  M src/pkg/encoding/binary/binary_test.go
>
>

[bradfitz, 2012-02-08 02:17:40]

LGTM
On Feb 8, 2012 1:13 PM, <r@golang.org> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> encoding/binary: hide TotalSize
>
> The function has a bizarre signature: it was the only public function
> there
> that exposed the reflect package. Also, its definition is peculiar and
> hard to
> explain. It doesn't merit being exported.
>
> This is an API change but really, it should never have been exported and
> it's certain very few programs will depend on it: it's too weird.
>
> Fixes issue 2846.
>
> Please review this at
http://codereview.appspot.com/**5639054/<http://codereview.appspot.com/5639054/>
>
> Affected files:
>  M doc/go1.html
>  M doc/go1.tmpl
>  M src/pkg/encoding/binary/**binary.go
>  M src/pkg/encoding/binary/**binary_test.go
>
>
>

[r, 2012-02-08 03:09:29]

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

encoding/binary: hide TotalSize

The function has a bizarre signature: it was the only public function there
that exposed the reflect package. Also, its definition is peculiar and hard to
explain. It doesn't merit being exported.

This is an API change but really, it should never have been exported and
it's certain very few programs will depend on it: it's too weird.

Fixes issue 2846.

R=golang-dev, gri, bradfitz
CC=golang-dev
http://codereview.appspot.com/5639054

[rog, 2012-02-08 10:41:15]

i'm not keen on this going.

it's nice to be able to know in advance how
many bytes will needed to for a given
fixed-size type, and i've used this functionality
in the past for writing header lengths.

i admit the signature was odd though - it
would be better if the argument was of type interface{].

i realise we can simulate it by writing an instance
of the type in question to a discarding writer,
but TotalSize seems more natrual.

On 8 February 2012 03:09,  <r@golang.org> wrote:
> *** Submitted as
> http://code.google.com/p/go/source/detail?r=8d1bf9e0a230 ***
>
>
> encoding/binary: hide TotalSize
>
> The function has a bizarre signature: it was the only public function
> there
> that exposed the reflect package. Also, its definition is peculiar and
> hard to
> explain. It doesn't merit being exported.
>
> This is an API change but really, it should never have been exported and
> it's certain very few programs will depend on it: it's too weird.
>
> Fixes issue 2846.
>
> R=golang-dev, gri, bradfitz
> CC=golang-dev
> http://codereview.appspot.com/5639054
>
>
> http://codereview.appspot.com/5639054/

[r2, 2012-02-08 11:34:17]

On 08/02/2012, at 9:41 PM, roger peppe wrote:

> i'm not keen on this going.
> 
> it's nice to be able to know in advance how
> many bytes will needed to for a given
> fixed-size type, and i've used this functionality
> in the past for writing header lengths.
> 
> i admit the signature was odd though - it
> would be better if the argument was of type interface{].
> 
> i realise we can simulate it by writing an instance
> of the type in question to a discarding writer,
> but TotalSize seems more natrual.

there was nothing natural about TotalSize. a better function might arise to take
its place after go 1.

-rob

[rsc, 2012-02-08 14:19:47]

If there is existing code that breaks because of
the removal of TotalSize, I think it would be fine to add

func Size(v interface{}) int {
    return dataSize(reflect.ValueOf(v))
}

Otherwise it can wait until after Go 1.

Russ