code review 148590043: context: fix TestAllocs to account for ints in interfaces

context: fix TestAllocs to account for ints in interfaces

Recent runtime changes mean that these are stored indirectly, requiring additional allocation.

[Sameer Ajmani, 2014-10-06 16:55:40]

Hello rsc@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-10-06 17:00:36]

https://codereview.appspot.com/148590043/diff/1/context/context.go
File context/context.go (right):

https://codereview.appspot.com/148590043/diff/1/context/context.go#newcode164
context/context.go:164: func (n emptyCtx) String() string {
why not here? then switch on *n.

https://codereview.appspot.com/148590043/diff/1/context/context.go#newcode184
context/context.go:184: return &background
I'd prefer you didn't modify these two and instead adjust the vars to be the
right type. Including making a little func to create them.

var (
      background = newEmptyCtx(1)
      todo     = newEmptyCtx(2)
)

func newEmptyContext(n emptyCtx) *emptyCtx { return &m }

[Sameer Ajmani, 2014-10-07 14:51:00]

https://codereview.appspot.com/148590043/diff/1/context/context.go
File context/context.go (right):

https://codereview.appspot.com/148590043/diff/1/context/context.go#newcode164
context/context.go:164: func (n emptyCtx) String() string {
On 2014/10/06 17:00:36, bradfitz wrote:
> why not here? then switch on *n.

Done, but switching on the pointers.

https://codereview.appspot.com/148590043/diff/1/context/context.go#newcode184
context/context.go:184: return &background
On 2014/10/06 17:00:35, bradfitz wrote:
> I'd prefer you didn't modify these two and instead adjust the vars to be the
> right type. Including making a little func to create them.
> 
> var (
>       background = newEmptyCtx(1)
>       todo     = newEmptyCtx(2)
> )
> 
> func newEmptyContext(n emptyCtx) *emptyCtx { return &m }
> 

Done.

[Sameer Ajmani, 2014-10-09 14:53:39]

Ping.  I'd like to get this in.

On Tue, Oct 7, 2014 at 10:50 AM, <sameer@golang.org> wrote:

>
> https://codereview.appspot.com/148590043/diff/1/context/context.go
> File context/context.go (right):
>
> https://codereview.appspot.com/148590043/diff/1/context/
> context.go#newcode164
> context/context.go:164: func (n emptyCtx) String() string {
> On 2014/10/06 17:00:36, bradfitz wrote:
>
>> why not here? then switch on *n.
>>
>
> Done, but switching on the pointers.
>
> https://codereview.appspot.com/148590043/diff/1/context/
> context.go#newcode184
> context/context.go:184: return &background
> On 2014/10/06 17:00:35, bradfitz wrote:
>
>> I'd prefer you didn't modify these two and instead adjust the vars to
>>
> be the
>
>> right type. Including making a little func to create them.
>>
>
>  var (
>>        background = newEmptyCtx(1)
>>        todo     = newEmptyCtx(2)
>> )
>>
>
>  func newEmptyContext(n emptyCtx) *emptyCtx { return &m }
>>
>
>
> Done.
>
> https://codereview.appspot.com/148590043/
>

[Sameer Ajmani, 2014-10-09 16:12:09]

Adding adononvan as a reviewer since both rsc and bradfitz are travelling.

On 2014/10/09 14:53:39, Sameer Ajmani wrote:
> Ping.  I'd like to get this in.
> 
> On Tue, Oct 7, 2014 at 10:50 AM, <mailto:sameer@golang.org> wrote:
> 
> >
> > https://codereview.appspot.com/148590043/diff/1/context/context.go
> > File context/context.go (right):
> >
> > https://codereview.appspot.com/148590043/diff/1/context/
> > context.go#newcode164
> > context/context.go:164: func (n emptyCtx) String() string {
> > On 2014/10/06 17:00:36, bradfitz wrote:
> >
> >> why not here? then switch on *n.
> >>
> >
> > Done, but switching on the pointers.
> >
> > https://codereview.appspot.com/148590043/diff/1/context/
> > context.go#newcode184
> > context/context.go:184: return &background
> > On 2014/10/06 17:00:35, bradfitz wrote:
> >
> >> I'd prefer you didn't modify these two and instead adjust the vars to
> >>
> > be the
> >
> >> right type. Including making a little func to create them.
> >>
> >
> >  var (
> >>        background = newEmptyCtx(1)
> >>        todo     = newEmptyCtx(2)
> >> )
> >>
> >
> >  func newEmptyContext(n emptyCtx) *emptyCtx { return &m }
> >>
> >
> >
> > Done.
> >
> > https://codereview.appspot.com/148590043/
> >

[adonovan, 2014-10-09 16:30:34]

On 2014/10/09 16:12:09, Sameer Ajmani wrote:
> Adding adononvan as a reviewer since both rsc and bradfitz are travelling.
> 
> On 2014/10/09 14:53:39, Sameer Ajmani wrote:
> > Ping.  I'd like to get this in.
> > 
> > On Tue, Oct 7, 2014 at 10:50 AM, <mailto:sameer@golang.org> wrote:
> > 
> > >
> > > https://codereview.appspot.com/148590043/diff/1/context/context.go
> > > File context/context.go (right):
> > >
> > > https://codereview.appspot.com/148590043/diff/1/context/
> > > context.go#newcode164
> > > context/context.go:164: func (n emptyCtx) String() string {
> > > On 2014/10/06 17:00:36, bradfitz wrote:
> > >
> > >> why not here? then switch on *n.
> > >>
> > >
> > > Done, but switching on the pointers.
> > >
> > > https://codereview.appspot.com/148590043/diff/1/context/
> > > context.go#newcode184
> > > context/context.go:184: return &background
> > > On 2014/10/06 17:00:35, bradfitz wrote:
> > >
> > >> I'd prefer you didn't modify these two and instead adjust the vars to
> > >>
> > > be the
> > >
> > >> right type. Including making a little func to create them.
> > >>
> > >
> > >  var (
> > >>        background = newEmptyCtx(1)
> > >>        todo     = newEmptyCtx(2)
> > >> )
> > >>
> > >
> > >  func newEmptyContext(n emptyCtx) *emptyCtx { return &m }
> > >>
> > >
> > >
> > > Done.
> > >
> > > https://codereview.appspot.com/148590043/
> > >

LGTM

Clarify CL description:
"Recent runtime changes mean that these are stored indirectly, requiring
additional allocation."

[adonovan, 2014-10-09 16:30:40]

https://codereview.appspot.com/148590043/diff/20001/context/context.go
File context/context.go (right):

https://codereview.appspot.com/148590043/diff/20001/context/context.go#newcod...
context/context.go:146: type emptyCtx int // not struct{} since we need distict
pointers
"distinct"

better:
// not struct{}, since all vars of this type must have distinct addresses

https://codereview.appspot.com/148590043/diff/20001/context/context.go#newcod...
context/context.go:176: return &e
Simplify:
  return new(emptyCtx)

It hardly seems worth having this func now that its body is a short expression.

[Sameer Ajmani, 2014-10-09 17:51:24]

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

context: fix TestAllocs to account for ints in interfaces

Recent runtime changes mean that these are stored indirectly, requiring
additional allocation.

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

[Sameer Ajmani, 2014-10-09 17:51:42]

https://codereview.appspot.com/148590043/diff/20001/context/context.go
File context/context.go (right):

https://codereview.appspot.com/148590043/diff/20001/context/context.go#newcod...
context/context.go:146: type emptyCtx int // not struct{} since we need distict
pointers
On 2014/10/09 16:30:39, adonovan wrote:
> "distinct"
> 
> better:
> // not struct{}, since all vars of this type must have distinct addresses

Done.

https://codereview.appspot.com/148590043/diff/20001/context/context.go#newcod...
context/context.go:176: return &e
On 2014/10/09 16:30:39, adonovan wrote:
> Simplify:
>   return new(emptyCtx)
> 
> It hardly seems worth having this func now that its body is a short
expression.

Done.