code review 5129057: gc: changes to export format in preparation for inlining.

gc: changes to export format in preparation for inlining.

string literals used as package qualifiers are now prefixed with '@'
which obviates the need for the extra ':' before tags.

[lvd1, 2011-09-28 00:33:29]

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

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

[gri, 2011-09-28 00:41:57]

FYI.

http://codereview.appspot.com/5129057/diff/4/src/pkg/go/types/gcimporter.go
File src/pkg/go/types/gcimporter.go (right):

http://codereview.appspot.com/5129057/diff/4/src/pkg/go/types/gcimporter.go#n...
src/pkg/go/types/gcimporter.go:719: // FuncBody = '{' ... '}'
s/'/"/ (4x)

http://codereview.appspot.com/5129057/diff/4/src/pkg/go/types/gcimporter.go#n...
src/pkg/go/types/gcimporter.go:733: // FuncDecl = "func" ExportedName Signature
.
... Signature [ FuncBody ] .

[rsc, 2011-09-28 18:43:57]

Looks pretty good.

http://codereview.appspot.com/5129057/diff/3011/src/cmd/gc/go.h
File src/cmd/gc/go.h (right):

http://codereview.appspot.com/5129057/diff/3011/src/cmd/gc/go.h#newcode1134
src/cmd/gc/go.h:1134: int	Vconv(Fmt *fp);
Please add a #pragma at the bottom of the file.

http://codereview.appspot.com/5129057/diff/3011/src/cmd/gc/go.y
File src/cmd/gc/go.y (right):

http://codereview.appspot.com/5129057/diff/3011/src/cmd/gc/go.y#newcode991
src/cmd/gc/go.y:991: |	'@' LLITERAL '.' LNAME
add at end of line  /* only during import */

Also, this same rule + action is duplicated below.
Maybe it should still be hidden_importsym.  Then the
comment is unnecessary, and this is just

sym:
    LNAME
|   hidden_importsym

http://codereview.appspot.com/5129057/diff/3011/src/cmd/gc/go.y#newcode1440
src/cmd/gc/go.y:1440: ntype
Can we keep name_or_type?

It's unnecessary in the grammar but it makes clear that
when you're parsing the argument list you don't yet know
whether this is a parameter name or a type.

http://codereview.appspot.com/5129057/diff/3011/src/cmd/gc/go.y#newcode1927
src/cmd/gc/go.y:1927: sym
I'd prefer if this stayed hidden_importsym.
Writing sym here suggests that a plain LNAME
is valid here, which is not true.
The explicit package name is fundamental to
the "set structpkg" part of hidden_pkg_importsym.

http://codereview.appspot.com/5129057/diff/3011/src/cmd/gc/subr.c
File src/cmd/gc/subr.c (right):

http://codereview.appspot.com/5129057/diff/3011/src/cmd/gc/subr.c#newcode766
src/cmd/gc/subr.c:766: if (n->val.ctype == CTxxx)
Odd to do this in the general case here.
Should be part of a new case OLITERAL.
Doesn't need to dodump the left and right.

http://codereview.appspot.com/5129057/diff/3011/src/cmd/gc/subr.c#newcode884
src/cmd/gc/subr.c:884: default:
if you move this default out of the switch
you don't need the unexplained return 0 below.

http://codereview.appspot.com/5129057/diff/3011/src/cmd/gc/subr.c#newcode1205
src/cmd/gc/subr.c:1205: // global flag packagequotes
Is this a note to yourself to explain the global?

http://codereview.appspot.com/5129057/diff/3011/src/pkg/go/types/gcimporter.go
File src/pkg/go/types/gcimporter.go (right):

http://codereview.appspot.com/5129057/diff/3011/src/pkg/go/types/gcimporter.g...
src/pkg/go/types/gcimporter.go:464: // TODO(gri) should this be
parseExportedName here?  YES(lvd)
Obviously delete this if you keep the code change.

http://codereview.appspot.com/5129057/diff/3011/test/ddd1.go
File test/ddd1.go (right):

http://codereview.appspot.com/5129057/diff/3011/test/ddd1.go#newcode19
test/ddd1.go:19: _ = sum([]int{1}) // ERROR "\[\]int.*as type int|incompatible"
I omitted the literals intentionally, because they can
be arbitrarily large and are probably not worth showing
in their entirety.  Can we have a short flag %#hN to format
as '[]int literal' instead?

http://codereview.appspot.com/5129057/diff/3011/test/escape2.go
File test/escape2.go (right):

http://codereview.appspot.com/5129057/diff/3011/test/escape2.go#newcode186
test/escape2.go:186: f := func() int { // ERROR "func .* escapes to heap"
Same here: is this printing the entire literal in the
error message?  That's problematic.  Should use %#hN and
write that as 'func literal'.

[lvd, 2011-10-01 21:20:10]

first round of responses.  will make the h flag for %#hN now.

http://codereview.appspot.com/5129057/diff/4/src/pkg/go/types/gcimporter.go
File src/pkg/go/types/gcimporter.go (right):

http://codereview.appspot.com/5129057/diff/4/src/pkg/go/types/gcimporter.go#n...
src/pkg/go/types/gcimporter.go:719: // FuncBody = '{' ... '}'
On 2011/09/28 00:41:57, gri wrote:
> s/'/"/ (4x)

Done.

http://codereview.appspot.com/5129057/diff/4/src/pkg/go/types/gcimporter.go#n...
src/pkg/go/types/gcimporter.go:733: // FuncDecl = "func" ExportedName Signature
.
On 2011/09/28 00:41:57, gri wrote:
> ... Signature [ FuncBody ] .

Done.

http://codereview.appspot.com/5129057/diff/3011/src/cmd/gc/go.h
File src/cmd/gc/go.h (right):

http://codereview.appspot.com/5129057/diff/3011/src/cmd/gc/go.h#newcode1134
src/cmd/gc/go.h:1134: int	Vconv(Fmt *fp);
On 2011/09/28 18:43:57, rsc wrote:
> Please add a #pragma at the bottom of the file.

Done.

http://codereview.appspot.com/5129057/diff/3011/src/cmd/gc/go.y
File src/cmd/gc/go.y (right):

http://codereview.appspot.com/5129057/diff/3011/src/cmd/gc/go.y#newcode991
src/cmd/gc/go.y:991: |	'@' LLITERAL '.' LNAME
On 2011/09/28 18:43:57, rsc wrote:
> add at end of line  /* only during import */
> 
> Also, this same rule + action is duplicated below.
> Maybe it should still be hidden_importsym.  Then the
> comment is unnecessary, and this is just
> 
> sym:
>     LNAME
> |   hidden_importsym

Done.

http://codereview.appspot.com/5129057/diff/3011/src/cmd/gc/go.y#newcode1440
src/cmd/gc/go.y:1440: ntype
On 2011/09/28 18:43:57, rsc wrote:
> Can we keep name_or_type?
> 
> It's unnecessary in the grammar but it makes clear that
> when you're parsing the argument list you don't yet know
> whether this is a parameter name or a type.

Done.

http://codereview.appspot.com/5129057/diff/3011/src/cmd/gc/go.y#newcode1927
src/cmd/gc/go.y:1927: sym
On 2011/09/28 18:43:57, rsc wrote:
> I'd prefer if this stayed hidden_importsym.
> Writing sym here suggests that a plain LNAME
> is valid here, which is not true.
> The explicit package name is fundamental to
> the "set structpkg" part of hidden_pkg_importsym.
> 

Done.

http://codereview.appspot.com/5129057/diff/3011/src/cmd/gc/subr.c
File src/cmd/gc/subr.c (right):

http://codereview.appspot.com/5129057/diff/3011/src/cmd/gc/subr.c#newcode766
src/cmd/gc/subr.c:766: if (n->val.ctype == CTxxx)
On 2011/09/28 18:43:57, rsc wrote:
> Odd to do this in the general case here.
> Should be part of a new case OLITERAL.
> Doesn't need to dodump the left and right.
> 

one of the reasons i added it was to see if there were other O's that had val
set, but if not then you're right.

http://codereview.appspot.com/5129057/diff/3011/src/cmd/gc/subr.c#newcode884
src/cmd/gc/subr.c:884: default:
On 2011/09/28 18:43:57, rsc wrote:
> if you move this default out of the switch
> you don't need the unexplained return 0 below.

Done.

http://codereview.appspot.com/5129057/diff/3011/src/cmd/gc/subr.c#newcode884
src/cmd/gc/subr.c:884: default:
On 2011/09/28 18:43:57, rsc wrote:
> if you move this default out of the switch
> you don't need the unexplained return 0 below.

Done.

http://codereview.appspot.com/5129057/diff/3011/src/cmd/gc/subr.c#newcode1205
src/cmd/gc/subr.c:1205: // global flag packagequotes
On 2011/09/28 18:43:57, rsc wrote:
> Is this a note to yourself to explain the global?

not just to myself

http://codereview.appspot.com/5129057/diff/3011/src/pkg/go/types/gcimporter.go
File src/pkg/go/types/gcimporter.go (right):

http://codereview.appspot.com/5129057/diff/3011/src/pkg/go/types/gcimporter.g...
src/pkg/go/types/gcimporter.go:464: // TODO(gri) should this be
parseExportedName here?  YES(lvd)
On 2011/09/28 18:43:57, rsc wrote:
> Obviously delete this if you keep the code change.

Done.

[lvd1, 2011-10-01 22:54:03]

On Sat, Oct 1, 2011 at 14:20, <lvd@google.com> wrote:

> ....  will make the h flag for %#hN now.


Of course, it was never going to be that easy.  exprfmt calls fmtprint,
which clears f->flags.  that's a problem when exprfmt recurses, eg for
&MyStruct, the call to fmtprint %O for the & leaves the flags cleared.  i
could change lib9/fmt/fmtprint.c to restore the flags on exit, (line 42)
because i wonder if that wasn't the intention.  however, i suspect this may
change expected outputs all over cmd/../*.c.  which needn't be disastrous.

Maybe it's time to bite the bullet and clean up the printing a bit b/c i've
wasted quite a bit of time walking through the call chains.  basically we
have 3 usecases for printing:  debugging the compiler,  errors and
diagnostics (-m), and printing the export section.
i could either go over all formats and make the + flag print end-user
friendly, and # in export format, or make a global flag that takes the role
of the existing erroring and exporting (or keep those) and make them used
consistently all over the place.

maybe there's a pragmatic shortcut, but it's not going to be prettier.

/L

[lvd, 2011-10-07 09:51:01]

PTAL

[lvd1, 2011-10-07 15:31:01]

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

Please take another look.

[rsc, 2011-10-07 15:59:50]

LGTM
But leaving go/types/* for gri.

http://codereview.appspot.com/5129057/diff/30002/src/cmd/gc/print.c
File src/cmd/gc/print.c (right):

http://codereview.appspot.com/5129057/diff/30002/src/cmd/gc/print.c#newcode510
src/cmd/gc/print.c:510: if (n->ninit) {
Why does this print as
{ x; if y { ... } }
instead of
if x; y { ... }
?

Maybe don't do that unless count(n->ninit) > 1?

http://codereview.appspot.com/5129057/diff/30002/src/cmd/gc/print.c#newcode527
src/cmd/gc/print.c:527: if (n->ninit) {
Same.

http://codereview.appspot.com/5129057/diff/30002/src/cmd/gc/print.c#newcode543
src/cmd/gc/print.c:543: if (n->ninit) {
Same.

http://codereview.appspot.com/5129057/diff/30002/src/cmd/gc/print.c#newcode559
src/cmd/gc/print.c:559: if (n->ninit) {
Same.

http://codereview.appspot.com/5129057/diff/30002/src/cmd/gc/subr.c
File src/cmd/gc/subr.c (right):

http://codereview.appspot.com/5129057/diff/30002/src/cmd/gc/subr.c#newcode886
src/cmd/gc/subr.c:886: return fmtprint(fp,  "I%B", v->u.xval);
I'd be happy to drop the prefixes (IFSB).  It is clear what the types are from
the format of the prints.

http://codereview.appspot.com/5129057/diff/30002/src/cmd/gc/subr.c#newcode1204
src/cmd/gc/subr.c:1204: // global flag packagequotes
Document what packagequotes means?

http://codereview.appspot.com/5129057/diff/30002/src/lib9/fmt/fmtprint.c
File src/lib9/fmt/fmtprint.c (right):

http://codereview.appspot.com/5129057/diff/30002/src/lib9/fmt/fmtprint.c#newc...
src/lib9/fmt/fmtprint.c:31: va_list va;
va_list va;
int n;
va_list va;

va_start(va, fmt);
n = fmtvprint(f, va);
va_end(va);
return n;

http://codereview.appspot.com/5129057/diff/30002/src/lib9/fmt/fmtvprint.c
File src/lib9/fmt/fmtvprint.c (right):

http://codereview.appspot.com/5129057/diff/30002/src/lib9/fmt/fmtvprint.c#new...
src/lib9/fmt/fmtvprint.c:38: fl = f->flags;
It is good to save these, but they also need to be cleared.
Add

f->flags = 0;
f->width = 0;
f->prec = 0;

[gri, 2011-10-07 17:50:24]

LGTM

http://codereview.appspot.com/5129057/diff/30002/src/pkg/go/types/gcimporter.go
File src/pkg/go/types/gcimporter.go (right):

http://codereview.appspot.com/5129057/diff/30002/src/pkg/go/types/gcimporter....
src/pkg/go/types/gcimporter.go:145: // Declare creates a named object of the
given kind in the scope.
s/creates/inserts/
s/in the/in/

http://codereview.appspot.com/5129057/diff/30002/src/pkg/go/types/gcimporter....
src/pkg/go/types/gcimporter.go:285: // 	scope := pkg.Data.(*ast.Scope)
this line should go

http://codereview.appspot.com/5129057/diff/30002/src/pkg/go/types/gcimporter....
src/pkg/go/types/gcimporter.go:462: // MethodSpec = { identifier | ExportedName
}  Signature .
It should be ( ) not { } . It's not repeated.

http://codereview.appspot.com/5129057/diff/30002/src/pkg/go/types/gcimporter....
src/pkg/go/types/gcimporter.go:723: // FuncBody = "{" ... "}"
nit pick:
// FuncBody = "{" ... "}" .
(missing period at end).

http://codereview.appspot.com/5129057/diff/30002/src/pkg/go/types/gcimporter....
src/pkg/go/types/gcimporter.go:737: // FuncDecl = "func" ExportedName Signature
[ FuncBody ].
nit pick: s/]./] ./

http://codereview.appspot.com/5129057/diff/30002/src/pkg/go/types/gcimporter....
src/pkg/go/types/gcimporter.go:750: // Receiver   = "(" ( identifier | "?" ) [
"*" ] ExportedName ")" [ FuncBody ].
nit pick: s/]./] ./

[lvd, 2011-10-08 16:40:03]

http://codereview.appspot.com/5129057/diff/30002/src/cmd/gc/print.c
File src/cmd/gc/print.c (right):

http://codereview.appspot.com/5129057/diff/30002/src/cmd/gc/print.c#newcode510
src/cmd/gc/print.c:510: if (n->ninit) {
On 2011/10/07 15:59:50, rsc wrote:
> Why does this print as
> { x; if y { ... } }
> instead of
> if x; y { ... }
> ?
> 
> Maybe don't do that unless count(n->ninit) > 1?

Done.

http://codereview.appspot.com/5129057/diff/30002/src/cmd/gc/print.c#newcode527
src/cmd/gc/print.c:527: if (n->ninit) {
On 2011/10/07 15:59:50, rsc wrote:
> Same.

Done.  there was a bug anyway, all three could have been empty.

http://codereview.appspot.com/5129057/diff/30002/src/cmd/gc/print.c#newcode543
src/cmd/gc/print.c:543: if (n->ninit) {
On 2011/10/07 15:59:50, rsc wrote:
> Same.

a range clause may not have an init, but we may have tacked some on anyway.

http://codereview.appspot.com/5129057/diff/30002/src/cmd/gc/print.c#newcode559
src/cmd/gc/print.c:559: if (n->ninit) {
On 2011/10/07 15:59:50, rsc wrote:
> Same.

Done.  selects can't have inits though.

http://codereview.appspot.com/5129057/diff/30002/src/cmd/gc/subr.c
File src/cmd/gc/subr.c (right):

http://codereview.appspot.com/5129057/diff/30002/src/cmd/gc/subr.c#newcode886
src/cmd/gc/subr.c:886: return fmtprint(fp,  "I%B", v->u.xval);
On 2011/10/07 15:59:50, rsc wrote:
> I'd be happy to drop the prefixes (IFSB).  It is clear what the types are from
> the format of the prints.

Done.

http://codereview.appspot.com/5129057/diff/30002/src/cmd/gc/subr.c#newcode1204
src/cmd/gc/subr.c:1204: // global flag packagequotes
On 2011/10/07 15:59:50, rsc wrote:
> Document what packagequotes means?

Done.

http://codereview.appspot.com/5129057/diff/30002/src/lib9/fmt/fmtprint.c
File src/lib9/fmt/fmtprint.c (right):

http://codereview.appspot.com/5129057/diff/30002/src/lib9/fmt/fmtprint.c#newc...
src/lib9/fmt/fmtprint.c:31: va_list va;
On 2011/10/07 15:59:50, rsc wrote:
> va_list va;
> int n;
> va_list va;
> 
> va_start(va, fmt);
> n = fmtvprint(f, va);
> va_end(va);
> return n;

Done.

http://codereview.appspot.com/5129057/diff/30002/src/lib9/fmt/fmtvprint.c
File src/lib9/fmt/fmtvprint.c (right):

http://codereview.appspot.com/5129057/diff/30002/src/lib9/fmt/fmtvprint.c#new...
src/lib9/fmt/fmtvprint.c:38: fl = f->flags;
On 2011/10/07 15:59:50, rsc wrote:
> It is good to save these, but they also need to be cleared.

dofmt/__fmtdispatch already does that for us (fmt.c line 162)

> Add
> 
> f->flags = 0;
> f->width = 0;
> f->prec = 0;

http://codereview.appspot.com/5129057/diff/30002/src/pkg/go/types/gcimporter.go
File src/pkg/go/types/gcimporter.go (right):

http://codereview.appspot.com/5129057/diff/30002/src/pkg/go/types/gcimporter....
src/pkg/go/types/gcimporter.go:145: // Declare creates a named object of the
given kind in the scope.
On 2011/10/07 17:50:24, gri wrote:
> s/creates/inserts/
> s/in the/in/

Done.

http://codereview.appspot.com/5129057/diff/30002/src/pkg/go/types/gcimporter....
src/pkg/go/types/gcimporter.go:285: // 	scope := pkg.Data.(*ast.Scope)
On 2011/10/07 17:50:24, gri wrote:
> this line should go

Doh.

http://codereview.appspot.com/5129057/diff/30002/src/pkg/go/types/gcimporter....
src/pkg/go/types/gcimporter.go:462: // MethodSpec = { identifier | ExportedName
}  Signature .
On 2011/10/07 17:50:24, gri wrote:
> It should be ( ) not { } . It's not repeated.

Done.

http://codereview.appspot.com/5129057/diff/30002/src/pkg/go/types/gcimporter....
src/pkg/go/types/gcimporter.go:737: // FuncDecl = "func" ExportedName Signature
[ FuncBody ].
On 2011/10/07 17:50:24, gri wrote:
> nit pick: s/]./] ./

Done.

http://codereview.appspot.com/5129057/diff/30002/src/pkg/go/types/gcimporter....
src/pkg/go/types/gcimporter.go:750: // Receiver   = "(" ( identifier | "?" ) [
"*" ] ExportedName ")" [ FuncBody ].
On 2011/10/07 17:50:24, gri wrote:
> nit pick: s/]./] ./

Done.

[lvd1, 2011-10-08 17:37:15]

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

gc: changes to export format in preparation for inlining.

string literals used as package qualifiers are now prefixed with '@'
which obviates the need for the extra ':' before tags.

R=rsc, gri, lvd
CC=golang-dev
http://codereview.appspot.com/5129057