code review 13535044: container/list: Dereferencing a list created with New()...

container/list: Dereferencing a list created with New() no longer breaks PushBack().

Caveat: Lists created with New() are no longer pre-initialized.

[Rym, 2013-09-12 01:14:07]

Hello golang-dev@googlegroups.com,

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

[bradfitz, 2013-09-12 09:12:35]

I don't think this is a bug.  Nobody expects to be able to do l2 =
*list.New(), do they?  This isn't a regression?

We're trying to get Go 1.2 out the door, though, so the tree is currently
frozen for non-critical changes.



On Thu, Sep 12, 2013 at 2:14 AM, <haqkrs@gmail.com> wrote:

> Reviewers: golang-dev1,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> container/list: Dereferencing a list created with New() no longer breaks
> PushBack().
>
> Caveat: Lists created with New() are no longer pre-initialized.
>
> Please review this at
https://codereview.appspot.**com/13535044/<https://codereview.appspot.com/135...
>
> Affected files (+31, -2 lines):
>   M src/pkg/container/list/list.go
>   M src/pkg/container/list/list_**test.go
>
>
> Index: src/pkg/container/list/list.go
> ==============================**==============================**=======
> --- a/src/pkg/container/list/list.**go
> +++ b/src/pkg/container/list/list.**go
> @@ -58,8 +58,8 @@
>         return l
>  }
>
> -// New returns an initialized list.
> -func New() *List { return new(List).Init() }
> +// New returns a lazily initialized list.
> +func New() *List { return new(List) }
>
>  // Len returns the number of elements of list l.
>  // The complexity is O(1).
> Index: src/pkg/container/list/list_**test.go
> ==============================**==============================**=======
> --- a/src/pkg/container/list/list_**test.go
> +++ b/src/pkg/container/list/list_**test.go
> @@ -141,6 +141,35 @@
>         checkListPointers(t, l, []*Element{})
>  }
>
> +func TestDereferencingNew(t *testing.T) {
> +       var l2 List = *New()
> +
> +       // Check PushFront()
> +       l2.PushFront("apple")
> +       l2.PushFront("pear")
> +
> +       if e := l2.Front(); e.Value != "pear" {
> +               t.Errorf("l2.Front().Value = %v, want %v", e.Value, "pear")
> +       }
> +
> +       if e := l2.Back(); e.Value != "apple" {
> +               t.Errorf("l2.Back().Value = %v, want %v", e.Value, "apple")
> +       }
> +
> +       // Check PushBack()
> +       l2 = *New()
> +       l2.PushBack("apple")
> +       l2.PushBack("pear")
> +
> +       if e := l2.Front(); e.Value != "apple" {
> +               t.Errorf("l2.Front().Value = %v, want %v", e.Value,
> "apple")
> +       }
> +
> +       if e := l2.Back(); e.Value != "pear" {
> +               t.Errorf("l2.Back().Value = %v, want %v", e.Value, "pear")
> +       }
> +}
> +
>  func checkList(t *testing.T, l *List, es []interface{}) {
>         if !checkListLen(t, l, len(es)) {
>                 return
>
>
> --
>
> ---You received this message because you are subscribed to the Google
> Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to
golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou...
> .
> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
> .
>

[Rym, 2013-09-16 00:34:23]

Well, I think it's unexpected behaviour, and I myself, expected to be able to do
so, it's valid syntax after all? 

I don't remember what reason I had, no good reason I'm sure, I just remember it
was working until it wasn't. I.e. it works fine for Front() then breaks
completely when you use Back(). 
If it's unsupported it shouldn't work at all not a half way confusing state,
it's rather funky that l2 = List{} works but not the above. 

Either way, it's absolutely non-critical so there's no reason at all to think
it's due for 1.2, if at all.

On 2013/09/12 09:12:35, bradfitz wrote:
> I don't think this is a bug.  Nobody expects to be able to do l2 =
> *list.New(), do they?  This isn't a regression?
> 
> We're trying to get Go 1.2 out the door, though, so the tree is currently
> frozen for non-critical changes.
> 
> 
> 
> On Thu, Sep 12, 2013 at 2:14 AM, <mailto:haqkrs@gmail.com> wrote:
> 
> > Reviewers: golang-dev1,
> >
> > Message:
> > Hello mailto:golang-dev@googlegroups.com,
> >
> > I'd like you to review this change to
> > https://code.google.com/p/go
> >
> >
> > Description:
> > container/list: Dereferencing a list created with New() no longer breaks
> > PushBack().
> >
> > Caveat: Lists created with New() are no longer pre-initialized.
> >
> > Please review this at
>
https://codereview.appspot.**com/13535044/%3Chttps://codereview.appspot.com/1...>
> >
> > Affected files (+31, -2 lines):
> >   M src/pkg/container/list/list.go
> >   M src/pkg/container/list/list_**test.go
> >
> >
> > Index: src/pkg/container/list/list.go
> > ==============================**==============================**=======
> > --- a/src/pkg/container/list/list.**go
> > +++ b/src/pkg/container/list/list.**go
> > @@ -58,8 +58,8 @@
> >         return l
> >  }
> >
> > -// New returns an initialized list.
> > -func New() *List { return new(List).Init() }
> > +// New returns a lazily initialized list.
> > +func New() *List { return new(List) }
> >
> >  // Len returns the number of elements of list l.
> >  // The complexity is O(1).
> > Index: src/pkg/container/list/list_**test.go
> > ==============================**==============================**=======
> > --- a/src/pkg/container/list/list_**test.go
> > +++ b/src/pkg/container/list/list_**test.go
> > @@ -141,6 +141,35 @@
> >         checkListPointers(t, l, []*Element{})
> >  }
> >
> > +func TestDereferencingNew(t *testing.T) {
> > +       var l2 List = *New()
> > +
> > +       // Check PushFront()
> > +       l2.PushFront("apple")
> > +       l2.PushFront("pear")
> > +
> > +       if e := l2.Front(); e.Value != "pear" {
> > +               t.Errorf("l2.Front().Value = %v, want %v", e.Value, "pear")
> > +       }
> > +
> > +       if e := l2.Back(); e.Value != "apple" {
> > +               t.Errorf("l2.Back().Value = %v, want %v", e.Value, "apple")
> > +       }
> > +
> > +       // Check PushBack()
> > +       l2 = *New()
> > +       l2.PushBack("apple")
> > +       l2.PushBack("pear")
> > +
> > +       if e := l2.Front(); e.Value != "apple" {
> > +               t.Errorf("l2.Front().Value = %v, want %v", e.Value,
> > "apple")
> > +       }
> > +
> > +       if e := l2.Back(); e.Value != "pear" {
> > +               t.Errorf("l2.Back().Value = %v, want %v", e.Value, "pear")
> > +       }
> > +}
> > +
> >  func checkList(t *testing.T, l *List, es []interface{}) {
> >         if !checkListLen(t, l, len(es)) {
> >                 return
> >
> >
> > --
> >
> > ---You received this message because you are subscribed to the Google
> > Groups "golang-dev" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> > email to
>
mailto:golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegroups.com>
> > .
> > For more options, visit
>
https://groups.google.com/**groups/opt_out%3Chttps://groups.google.com/groups...>
> > .
> >

[rsc, 2013-09-23 17:36:22]

not lgtm

[rsc, 2013-12-18 20:12:52]

R=close