code review 6543057: cmd/gc: Don't export embedded builtins

cmd/gc: Don't export embedded builtins

Fixes issue 4124.

[DMorsing, 2012-09-22 09:43:58]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com,
rsc@golang.org),

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

[DMorsing, 2012-09-22 09:45:29]

I think I got this right, but I'm very unsure. Could everyone please that a
closer look than normal?

[dfc, 2012-09-22 09:59:51]

Fixes issue 4124

Also, I think you should include the test code from issue 4124 in
addition to your reflect example.

On Sat, Sep 22, 2012 at 7:45 PM,  <daniel.morsing@gmail.com> wrote:
> I think I got this right, but I'm very unsure. Could everyone please
> that a closer look than normal?
>
>
> http://codereview.appspot.com/6543057/

[DMorsing, 2012-09-22 11:18:32]

On 2012/09/22 09:59:51, dfc wrote:
> Fixes issue 4124

Whoops. Added now
> 
> Also, I think you should include the test code from issue 4124 in
> addition to your reflect example.

I tried, but I'd have to use compiledir and that can't handle compilations that
fails.

[rsc, 2012-09-22 12:51:23]

The test using reflect is nice, but can you also make a test checking
the behavior at compile time? Strictly speaking there's no guarantee
the two would be in sync. It will have to be a 2-file 'rundir' test
like the one you found before.

Russ

[DMorsing, 2012-09-22 14:26:31]

On Sat, Sep 22, 2012 at 2:51 PM, Russ Cox <rsc@golang.org> wrote:
> The test using reflect is nice, but can you also make a test checking
> the behavior at compile time? Strictly speaking there's no guarantee
> the two would be in sync. It will have to be a 2-file 'rundir' test
> like the one you found before.
>

compiledir only checks that the compile is successful. Since I have to
do something like errorcheckdir, I think I have to change the test
framework in order to test that part of this change.

[rsc, 2012-09-22 15:03:08]

> compiledir only checks that the compile is successful. Since I have to
> do something like errorcheckdir, I think I have to change the test
> framework in order to test that part of this change.

indeed. sorry about that. if you're going to do that could you also
add rundir while you're there (issue 4058)? thanks.

[DMorsing, 2012-09-22 15:30:32]

On Sat, Sep 22, 2012 at 5:03 PM, Russ Cox <rsc@golang.org> wrote:
>> compiledir only checks that the compile is successful. Since I have to
>> do something like errorcheckdir, I think I have to change the test
>> framework in order to test that part of this change.
>
> indeed. sorry about that. if you're going to do that could you also
> add rundir while you're there (issue 4058)? thanks.

Will do. Do I need to add it to testlib as well?

[minux1, 2012-09-22 15:36:45]

On Saturday, September 22, 2012, Daniel Morsing wrote:

> On Sat, Sep 22, 2012 at 5:03 PM, Russ Cox <rsc@golang.org <javascript:;>>
> wrote:
> > indeed. sorry about that. if you're going to do that could you also
> > add rundir while you're there (issue 4058)? thanks.
> Will do. Do I need to add it to testlib as well?
>
Please do so. test/run should still be able to do all the tests
(some tests are still runnable only by testlib)

[DMorsing, 2012-10-06 08:42:32]

PTAL.

Added a compilation test that uses the new errorcheckdir functionality in the
test framework.

[remyoudompheng, 2012-10-06 16:51:09]

Does it happen to fix issue 3552?

[DMorsing, 2012-10-06 18:38:39]

On 2012/10/06 16:51:09, remyoudompheng wrote:
> Does it happen to fix issue 3552?

Quick test suggests no.

[rsc, 2012-10-06 22:25:56]

LGTM

[DMorsing, 2012-10-07 04:54:04]

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

cmd/gc: Don't export embedded builtins

Fixes issue 4124.

R=golang-dev, dave, minux.ma, remyoudompheng, rsc
CC=golang-dev
http://codereview.appspot.com/6543057

[DMorsing, 2012-10-07 04:59:29]

Little postscript to this CL. Should we change the naming convention for files
in the fixed bugs directory? I had to rename my tests twice because someone had
already taken the name.

I know of at least one test which got overwritten because of this problem, but I
can't find a reference to it.

[minux1, 2012-10-07 05:48:07]

On Sun, Oct 7, 2012 at 12:59 PM, <daniel.morsing@gmail.com> wrote:

> Little postscript to this CL. Should we change the naming convention for
> files in the fixed bugs directory? I had to rename my tests twice
> because someone had already taken the name.
>
Yeah, the same happened to me twice.
I propose we use something like test/fixedissues/issueNNN.go.

>
> I know of at least one test which got overwritten because of this
> problem, but I can't find a reference to it.
>
http://codereview.appspot.com/6443100

[rsc, 2012-10-07 14:03:29]

I don't want to create a new directory, but I don't mind
test/fixedbugs/issueNNNN for this.