compiler: Don't insert promoted methods that conflict with fields.

compiler: Don't insert promoted methods that conflict with fields.

[timshen, 2014-09-28 05:55:20]

Hello gofrontend-dev@googlegroups.com,

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

[timshen, 2014-09-28 06:12:30]

On Sat, Sep 27, 2014 at 10:55 PM,  <timshen@google.com> wrote:
> Reviewers: gofrontend-dev_googlegroups.com,
>
> Message:
> Hello gofrontend-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://code.google.com/p/gofrontend/

Hi,

This is my first CL for gofrontend :)

I'm not sure if Methods::remove causes a memory leak, since it's too
hard to track (and blindly deleting the removed element will cause a
segfault) ------ valgrind tells me there're other leaks in gofrontend.
Is that a problem?

By the way, where should I add the testcase?

Thanks!


-- 
Regards,
Tim Shen

[iant, 2014-09-28 15:09:54]

Thanks for working on this.

The first line of the CL description should describe what the change does. 
Something like

compiler: Remove promoted methods that conflict with fields.

See the existing descriptions.

The test case should be added to the gc testsuite if it is not already there.

But I'm not sure this is the right approach.  Rather than go back and remove
conflicting methods, I don't think we should ever have added them in the first
place.

Your patch just looks at the top level of fields; I'm concerned that it won't do
the right thing if the struct with the conflicting field/method is itself
embedded in yet another struct.  I think the promoted method will bubble up, and
we won't notice because we won't be looking at the fields of the embedded
struct.

https://codereview.appspot.com/147280043/diff/60001/go/types.cc
File go/types.cc (right):

https://codereview.appspot.com/147280043/diff/60001/go/types.cc#newcode8612
go/types.cc:8612: // Check if there're prompted methods that conflict with field
names and
s/promoted/promoted/

[timshen, 2014-09-28 23:36:24]

On Sun, Sep 28, 2014 at 8:09 AM,  <iant@golang.org> wrote:
> Thanks for working on this.
>
> The first line of the CL description should describe what the change
> does.  Something like
>
> compiler: Remove promoted methods that conflict with fields.
>
> See the existing descriptions.

Done.

> The test case should be added to the gc testsuite if it is not already
> there.

So the test case goes first; after it's mirrored to gcc, submit this
CL, I suppose?

> But I'm not sure this is the right approach.  Rather than go back and
> remove conflicting methods, I don't think we should ever have added them
> in the first place.
>
> Your patch just looks at the top level of fields; I'm concerned that it
> won't do the right thing if the struct with the conflicting field/method
> is itself embedded in yet another struct.  I think the promoted method
> will bubble up, and we won't notice because we won't be looking at the
> fields of the embedded struct.

You are right. Put it into the recursion function.

By the way, valgrind detects some memory leak (before this CL). Is it normal?


-- 
Regards,
Tim Shen

[iant, 2014-09-29 02:29:59]

On Sun, Sep 28, 2014 at 4:36 PM, 'Tim Shen' via gofrontend-dev
<gofrontend-dev@googlegroups.com> wrote:
> On Sun, Sep 28, 2014 at 8:09 AM,  <iant@golang.org> wrote:
>
>> The test case should be added to the gc testsuite if it is not already
>> there.
>
> So the test case goes first; after it's mirrored to gcc, submit this
> CL, I suppose?

No, it's fine to submit this CL before the test gets over to the gccgo
repository, as long as the test makes it into the gc repository at
some point.  The important thing is to make sure we don't regress in
the future.


> By the way, valgrind detects some memory leak (before this CL). Is it normal?

Yes, there are lots of leaks in the gofrontend code.  It's a compiler,
not a server; memory leaks are not good but they aren't huge problems.
My somewhat vague thinking has been that we will eventually change the
standard types in the gofrontend to use a custom allocator, and simply
drop all the allocated memory after handing everything over to the
middle end.

Ian

[timshen, 2014-09-29 06:04:33]

On Sun, Sep 28, 2014 at 7:29 PM, Ian Lance Taylor <iant@golang.org> wrote:
> No, it's fine to submit this CL before the test gets over to the gccgo
> repository, as long as the test makes it into the gc repository at
> some point.  The important thing is to make sure we don't regress in
> the future.

That's great :)

Is the latest change OK? I intentionally put the new and delete
together in Type::finalize_methods to prevent potential leak.


-- 
Regards,
Tim Shen

[iant, 2014-10-03 00:54:30]

Looks pretty good.

The first line of the CL description should describe what the change does. 
Something like

compiler: Remove promoted methods that conflict with fields.

You can change this by using "hg change".

https://codereview.appspot.com/147280043/diff/80001/go/types.cc
File go/types.cc (right):

https://codereview.appspot.com/147280043/diff/80001/go/types.cc#newcode9543
go/types.cc:9543: // Check if there're promoted methods that conflict with field
names and
s/there're/there are/

https://codereview.appspot.com/147280043/diff/80001/go/types.h
File go/types.h (right):

https://codereview.appspot.com/147280043/diff/80001/go/types.h#newcode389
go/types.h:389: { return methods_.empty(); }
Use this->methods_.

[timshen, 2014-10-03 04:44:56]

On Thu, Oct 2, 2014 at 5:54 PM,  <iant@golang.org> wrote:
> You can change this by using "hg change".

I suppose it's "compiler: Don't insert promoted methods that conflict
with fields." now. I did it through codereview website.

> https://codereview.appspot.com/147280043/diff/80001/go/types.cc
> File go/types.cc (right):
>
> https://codereview.appspot.com/147280043/diff/80001/go/types.cc#newcode9543
> go/types.cc:9543: // Check if there're promoted methods that conflict
> with field names and
> s/there're/there are/

Done.

> https://codereview.appspot.com/147280043/diff/80001/go/types.h
> File go/types.h (right):
>
> https://codereview.appspot.com/147280043/diff/80001/go/types.h#newcode389
> go/types.h:389: { return methods_.empty(); }
> Use this->methods_.

Done.


-- 
Regards,
Tim Shen

[iant, 2014-10-03 15:11:06]

LGTM

Thanks.

[iant, 2014-10-03 15:13:22]

*** Submitted as
https://code.google.com/p/gofrontend/source/detail?r=9a88882ca32c ***

compiler: Don't insert promoted methods that conflict with fields.

LGTM=iant
R=gofrontend-dev, iant
CC=gofrontend-dev
https://codereview.appspot.com/147280043

Committer: Ian Lance Taylor <iant@golang.org>