code review 5316043: gc: clean up printing.

gc: clean up printing.

Got rid of all the magic mystery globals. Now
for %N, %T, and %S, the flags +,- and # set a sticky
debug, sym and export mode, only visible in the new fmt.c.
Default is error mode. Handle h and l flags consistently with
the least side effects, so we can now change
things without worrying about unrelated things
breaking.

fixes issue 2361

[lvd, 2011-10-19 20:18:23]

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

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

[lvd, 2011-10-21 15:48:02]

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

Please take another look.

[lvd, 2011-10-25 15:17:12]

i'm currently adding everything i find while working on 5296053, but don't
let that stop you from reviewing anything.
/L

On Fri, Oct 21, 2011 at 17:48, <lvd@google.com> wrote:

> Hello rsc@golang.org (cc: golang-dev@googlegroups.com),
>
> Please take another look.
>
>
>
http://codereview.appspot.com/**5316043/<http://codereview.appspot.com/5316043/>
>

[rsc, 2011-10-26 22:28:36]

Thanks very much.  It's definitely an improvement.
A bunch of small things below.

I suspect that the handling of bytetype (and you need to add runetype now)
is wrong across imports, but there was no test in the tree for that.
I just added one: hg sync to get it.

In general, the distinction between byte and uint8 (and rune and int)
should always be preserved except in one case: the print inside
reflect.c's typesym must collapse the two so that it ends up mapping
both byte and uint8 to the same reflect type symbol.  Dynamic interface
checks will fail at runtime if not.

http://codereview.appspot.com/5316043/diff/2003/src/cmd/gc/dcl.c
File src/cmd/gc/dcl.c (right):

http://codereview.appspot.com/5316043/diff/2003/src/cmd/gc/dcl.c#newcode1092
src/cmd/gc/dcl.c:1092: //print("methodsym: %s\n", p);
delete?

http://codereview.appspot.com/5316043/diff/2003/src/cmd/gc/export.c
File src/cmd/gc/export.c (right):

http://codereview.appspot.com/5316043/diff/2003/src/cmd/gc/export.c#newcode86
src/cmd/gc/export.c:86: if(!p->direct)	// TODO(lvd) does this ever happen?
Yes.  cd $GOROOT/pkg/$GOOS_GOARCH
grep -a '// indirect' *.a

http://codereview.appspot.com/5316043/diff/2003/src/cmd/gc/export.c#newcode339
src/cmd/gc/export.c:339: yyerror("inconsistent definition for var %S during
import\n\t%T\n\t%T",	s, s->def->type, t);
drop tab before s,

http://codereview.appspot.com/5316043/diff/2003/src/cmd/gc/export.c#newcode356
src/cmd/gc/export.c:356: print("import type %T %T\n", pt, t);
Why is this %T now instead of %lT?

http://codereview.appspot.com/5316043/diff/2003/src/cmd/gc/fmt.c
File src/cmd/gc/fmt.c (right):

http://codereview.appspot.com/5316043/diff/2003/src/cmd/gc/fmt.c#newcode5
src/cmd/gc/fmt.c:5: // Useful format combinations:
This list is showing all possible combinations instead of telling me
the definitions of the individual pieces.  Can we do the latter instead?

// Formatting verbs.
//
// %H - NodeList*
//   flag +: recursive multiline debug dump
//   flag h: shorten debug dump to single line
//
// %S - Sym* (x or p.x)
//   flag +: debug mode (example)
//   flag #: export mode (example)
//
// %T - Type* (int or chan struct{})
//   flag l: show definition even for named types
//   flag h: omit "func (rcvr)" for method functions
//   flag #: export mode (example)

etc.  The list should be complete, and sorted by letter: J L O V all belong here
too.

http://codereview.appspot.com/5316043/diff/2003/src/cmd/gc/fmt.c#newcode15
src/cmd/gc/fmt.c:15: //	%#lT		type definition instead of name
Why do I need a # here?

http://codereview.appspot.com/5316043/diff/2003/src/cmd/gc/fmt.c#newcode16
src/cmd/gc/fmt.c:16: //	%#hT		omit"func" and receiver in function signature
Same.

http://codereview.appspot.com/5316043/diff/2003/src/cmd/gc/fmt.c#newcode29
src/cmd/gc/fmt.c:29: // mode inside the printing functions (eg yyerror inside an
export dump).
If I call yyerror inside an export dump, can't it just work?
I don't want to have to think about whether this yyerror statement
is inside an export dump or not.

http://codereview.appspot.com/5316043/diff/2003/src/cmd/gc/fmt.c#newcode318
src/cmd/gc/fmt.c:318: // Ftm "%V": Values
Fmt

http://codereview.appspot.com/5316043/diff/2003/src/cmd/gc/fmt.c#newcode450
src/cmd/gc/fmt.c:450: Sprint(Fmt *fp, Sym *s)
symfmt

http://codereview.appspot.com/5316043/diff/2003/src/cmd/gc/fmt.c#newcode510
src/cmd/gc/fmt.c:510: Tprint(Fmt *fp, Type *t)
typefmt

http://codereview.appspot.com/5316043/diff/2003/src/cmd/gc/fmt.c#newcode517
src/cmd/gc/fmt.c:517: // Distinguish byte from uint8 only in error and debug
mode.
They are supposed to be distinguished in export mode too.
The only time it is not supposed to be distinguished is when
preparing the names and strings that are used for reflect symbols.

http://codereview.appspot.com/5316043/diff/2003/src/cmd/gc/fmt.c#newcode544
src/cmd/gc/fmt.c:544: // TBD roll into switch below as default: fmtprint(fp,
"%hS", t->sym)?
Doesn't seem worth it.  The bottom of the function isn't in a switch default
either.  Not all of these have syms either.

http://codereview.appspot.com/5316043/diff/2003/src/cmd/gc/fmt.c#newcode546
src/cmd/gc/fmt.c:546: if(0 && (t == idealbool || t == idealstring))	// who wants
to see 'ideal'?
I do, in error messages.

http://codereview.appspot.com/5316043/diff/2003/src/cmd/gc/fmt.c#newcode684
src/cmd/gc/fmt.c:684: int mi;
int doinit, extrablock;

doinit = 0;
extrablock = 0;

http://codereview.appspot.com/5316043/diff/2003/src/cmd/gc/fmt.c#newcode696
src/cmd/gc/fmt.c:696: mi = 1;
    extrablock = 1;
else if (n->ninit)
    doinit = 1;

http://codereview.appspot.com/5316043/diff/2003/src/cmd/gc/fmt.c#newcode700
src/cmd/gc/fmt.c:700: mi = 1;
extrablock = 1

http://codereview.appspot.com/5316043/diff/2003/src/cmd/gc/fmt.c#newcode720
src/cmd/gc/fmt.c:720: if (n->colas)
,s/if (/if(/g
,s/switch (/switch(/g
,s/for (/for(/g

http://codereview.appspot.com/5316043/diff/2003/src/cmd/gc/fmt.c#newcode757
src/cmd/gc/fmt.c:757: if(!mi && n->ninit)
if(doinit)

http://codereview.appspot.com/5316043/diff/2003/src/cmd/gc/fmt.c#newcode762
src/cmd/gc/fmt.c:762: fmtprint(f, "else { %H }", n->nelse);
s/"else/" else/

http://codereview.appspot.com/5316043/diff/2003/src/cmd/gc/fmt.c#newcode770
src/cmd/gc/fmt.c:770: if (!mi && n->ninit && n->nincr)
if(doinit && n->nincr)
else if(n->nincr)
else if(doinit)
else

http://codereview.appspot.com/5316043/diff/2003/src/cmd/gc/fmt.c#newcode794
src/cmd/gc/fmt.c:794: if(!mi && n->ninit && n->ntest)
if(doinit && n->ntest)
else if(n->ntest)
else

http://codereview.appspot.com/5316043/diff/2003/src/cmd/gc/fmt.c#newcode1037
src/cmd/gc/fmt.c:1037: if (n->left && n->right)
It's okay to call fmtprint multiple times.

if(n->left)
    fmtprint(f, "%hN", n->left)
fmtprint(f, ":")
if(n->right)
    fmtprint(f, "%N", n->right)

It's not obvious to me why left gets h but right does not.

http://codereview.appspot.com/5316043/diff/2003/src/cmd/gc/fmt.c#newcode1158
src/cmd/gc/fmt.c:1158: Nprint(Fmt *f, Node *n)
nodefmt

http://codereview.appspot.com/5316043/diff/2003/src/cmd/gc/fmt.c#newcode1189
src/cmd/gc/fmt.c:1189: Ndump(Fmt *fp, Node *n)
nodedump

http://codereview.appspot.com/5316043/diff/2003/src/cmd/gc/lex.c
File src/cmd/gc/lex.c (right):

http://codereview.appspot.com/5316043/diff/2003/src/cmd/gc/lex.c#newcode198
src/cmd/gc/lex.c:198: fmtinstall('B', Bconv);	// big numbers
Move these into fmtinstallgo.

[lvd, 2011-10-28 10:57:50]

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

Please take another look.

[lvd, 2011-10-28 11:02:57]

hm. linker not happy about Byte. yet, for the rest all done.

http://codereview.appspot.com/5316043/diff/2003/src/cmd/gc/dcl.c
File src/cmd/gc/dcl.c (right):

http://codereview.appspot.com/5316043/diff/2003/src/cmd/gc/dcl.c#newcode1092
src/cmd/gc/dcl.c:1092: //print("methodsym: %s\n", p);
On 2011/10/26 22:28:36, rsc wrote:
> delete?

Done.

http://codereview.appspot.com/5316043/diff/2003/src/cmd/gc/export.c
File src/cmd/gc/export.c (right):

http://codereview.appspot.com/5316043/diff/2003/src/cmd/gc/export.c#newcode86
src/cmd/gc/export.c:86: if(!p->direct)	// TODO(lvd) does this ever happen?
On 2011/10/26 22:28:36, rsc wrote:
> Yes.  cd $GOROOT/pkg/$GOOS_GOARCH
> grep -a '// indirect' *.a

ah didn't see it before, but that must have been with an incomplete build.

http://codereview.appspot.com/5316043/diff/2003/src/cmd/gc/export.c#newcode339
src/cmd/gc/export.c:339: yyerror("inconsistent definition for var %S during
import\n\t%T\n\t%T",	s, s->def->type, t);
On 2011/10/26 22:28:36, rsc wrote:
> drop tab before s,

Done.

http://codereview.appspot.com/5316043/diff/2003/src/cmd/gc/export.c#newcode339
src/cmd/gc/export.c:339: yyerror("inconsistent definition for var %S during
import\n\t%T\n\t%T",	s, s->def->type, t);
On 2011/10/26 22:28:36, rsc wrote:
> drop tab before s,

Done.

http://codereview.appspot.com/5316043/diff/2003/src/cmd/gc/export.c#newcode356
src/cmd/gc/export.c:356: print("import type %T %T\n", pt, t);
On 2011/10/26 22:28:36, rsc wrote:
> Why is this %T now instead of %lT?

because if you have type foo int63; type bar foo; this will print that back,
otherwise you'd see type bar int63 for the second.

this goes back to a bit of confusion between things and names for things in the
design.  a similar thing happens for constants.

http://codereview.appspot.com/5316043/diff/2003/src/cmd/gc/fmt.c
File src/cmd/gc/fmt.c (right):

http://codereview.appspot.com/5316043/diff/2003/src/cmd/gc/fmt.c#newcode5
src/cmd/gc/fmt.c:5: // Useful format combinations:
On 2011/10/26 22:28:36, rsc wrote:
> This list is showing all possible combinations instead of telling me
> the definitions of the individual pieces.  Can we do the latter instead?
> 

yes, i was saving that until it was finished. 

> // Formatting verbs.
> //
> // %H - NodeList*
> //   flag +: recursive multiline debug dump
> //   flag h: shorten debug dump to single line
> //
> // %S - Sym* (x or p.x)
> //   flag +: debug mode (example)
> //   flag #: export mode (example)
> //
> // %T - Type* (int or chan struct{})
> //   flag l: show definition even for named types
> //   flag h: omit "func (rcvr)" for method functions
> //   flag #: export mode (example)
> 
> etc.  The list should be complete, and sorted by letter: J L O V all belong
here
> too.

http://codereview.appspot.com/5316043/diff/2003/src/cmd/gc/fmt.c#newcode29
src/cmd/gc/fmt.c:29: // mode inside the printing functions (eg yyerror inside an
export dump).
On 2011/10/26 22:28:36, rsc wrote:
> If I call yyerror inside an export dump, can't it just work?
> I don't want to have to think about whether this yyerror statement
> is inside an export dump or not.

note that yyerror during any of the functions in export.c works fine, it's just
for yyerrors inside the Nprint below. turns out i eliminated the only occurence
of that, so it doesn't really matter.

i could have yyerror switch the default of course, but that would open up the
hidden global mode to the world again (no big deal).  how 'bout we do that when
the first instance of this appears.

http://codereview.appspot.com/5316043/diff/2003/src/cmd/gc/fmt.c#newcode318
src/cmd/gc/fmt.c:318: // Ftm "%V": Values
On 2011/10/26 22:28:36, rsc wrote:
> Fmt

Done.

http://codereview.appspot.com/5316043/diff/2003/src/cmd/gc/fmt.c#newcode450
src/cmd/gc/fmt.c:450: Sprint(Fmt *fp, Sym *s)
On 2011/10/26 22:28:36, rsc wrote:
> symfmt

Done.

http://codereview.appspot.com/5316043/diff/2003/src/cmd/gc/fmt.c#newcode510
src/cmd/gc/fmt.c:510: Tprint(Fmt *fp, Type *t)
On 2011/10/26 22:28:36, rsc wrote:
> typefmt

Done.

http://codereview.appspot.com/5316043/diff/2003/src/cmd/gc/fmt.c#newcode517
src/cmd/gc/fmt.c:517: // Distinguish byte from uint8 only in error and debug
mode.
On 2011/10/26 22:28:36, rsc wrote:
> They are supposed to be distinguished in export mode too.
> The only time it is not supposed to be distinguished is when
> preparing the names and strings that are used for reflect symbols.

Done.

http://codereview.appspot.com/5316043/diff/2003/src/cmd/gc/fmt.c#newcode517
src/cmd/gc/fmt.c:517: // Distinguish byte from uint8 only in error and debug
mode.
On 2011/10/26 22:28:36, rsc wrote:
> They are supposed to be distinguished in export mode too.
> The only time it is not supposed to be distinguished is when
> preparing the names and strings that are used for reflect symbols.

Done.

http://codereview.appspot.com/5316043/diff/2003/src/cmd/gc/fmt.c#newcode544
src/cmd/gc/fmt.c:544: // TBD roll into switch below as default: fmtprint(fp,
"%hS", t->sym)?
On 2011/10/26 22:28:36, rsc wrote:
> Doesn't seem worth it.  The bottom of the function isn't in a switch default
> either.  Not all of these have syms either.
> 

Done.

http://codereview.appspot.com/5316043/diff/2003/src/cmd/gc/fmt.c#newcode546
src/cmd/gc/fmt.c:546: if(0 && (t == idealbool || t == idealstring))	// who wants
to see 'ideal'?
On 2011/10/26 22:28:36, rsc wrote:
> I do, in error messages.

Done.

http://codereview.appspot.com/5316043/diff/2003/src/cmd/gc/fmt.c#newcode696
src/cmd/gc/fmt.c:696: mi = 1;
On 2011/10/26 22:28:36, rsc wrote:
>     extrablock = 1;
> else if (n->ninit)
>     doinit = 1;

Done.

http://codereview.appspot.com/5316043/diff/2003/src/cmd/gc/fmt.c#newcode700
src/cmd/gc/fmt.c:700: mi = 1;
On 2011/10/26 22:28:36, rsc wrote:
> extrablock = 1

Done.

http://codereview.appspot.com/5316043/diff/2003/src/cmd/gc/fmt.c#newcode720
src/cmd/gc/fmt.c:720: if (n->colas)
On 2011/10/26 22:28:36, rsc wrote:
> ,s/if (/if(/g
> ,s/switch (/switch(/g
> ,s/for (/for(/g

Done.

http://codereview.appspot.com/5316043/diff/2003/src/cmd/gc/fmt.c#newcode757
src/cmd/gc/fmt.c:757: if(!mi && n->ninit)
On 2011/10/26 22:28:36, rsc wrote:
> if(doinit)

Done.

http://codereview.appspot.com/5316043/diff/2003/src/cmd/gc/fmt.c#newcode762
src/cmd/gc/fmt.c:762: fmtprint(f, "else { %H }", n->nelse);
On 2011/10/26 22:28:36, rsc wrote:
> s/"else/" else/

Done.

http://codereview.appspot.com/5316043/diff/2003/src/cmd/gc/fmt.c#newcode770
src/cmd/gc/fmt.c:770: if (!mi && n->ninit && n->nincr)
On 2011/10/26 22:28:36, rsc wrote:
> if(doinit && n->nincr)
> else if(n->nincr)
> else if(doinit)
> else

Done.

http://codereview.appspot.com/5316043/diff/2003/src/cmd/gc/fmt.c#newcode770
src/cmd/gc/fmt.c:770: if (!mi && n->ninit && n->nincr)
On 2011/10/26 22:28:36, rsc wrote:
> if(doinit && n->nincr)
> else if(n->nincr)
> else if(doinit)
> else

Done.  and handle the case of empty ntest.

http://codereview.appspot.com/5316043/diff/2003/src/cmd/gc/fmt.c#newcode794
src/cmd/gc/fmt.c:794: if(!mi && n->ninit && n->ntest)
On 2011/10/26 22:28:36, rsc wrote:
> if(doinit && n->ntest)
> else if(n->ntest)
> else

Done.

http://codereview.appspot.com/5316043/diff/2003/src/cmd/gc/fmt.c#newcode1037
src/cmd/gc/fmt.c:1037: if (n->left && n->right)
On 2011/10/26 22:28:36, rsc wrote:
> It's okay to call fmtprint multiple times.
> 

I find this clearer b/c you can tell in one view what the whole node is going to
look like.

> if(n->left)
>     fmtprint(f, "%hN", n->left)
> fmtprint(f, ":")
> if(n->right)
>     fmtprint(f, "%N", n->right)
> 
> It's not obvious to me why left gets h but right does not.

leftover from worring about struct literals, for which the key (left) is always
a field name.  gone now.

http://codereview.appspot.com/5316043/diff/2003/src/cmd/gc/fmt.c#newcode1158
src/cmd/gc/fmt.c:1158: Nprint(Fmt *f, Node *n)
On 2011/10/26 22:28:36, rsc wrote:
> nodefmt

Done.

http://codereview.appspot.com/5316043/diff/2003/src/cmd/gc/fmt.c#newcode1189
src/cmd/gc/fmt.c:1189: Ndump(Fmt *fp, Node *n)
On 2011/10/26 22:28:36, rsc wrote:
> nodedump

Done.

http://codereview.appspot.com/5316043/diff/2003/src/cmd/gc/lex.c
File src/cmd/gc/lex.c (right):

http://codereview.appspot.com/5316043/diff/2003/src/cmd/gc/lex.c#newcode198
src/cmd/gc/lex.c:198: fmtinstall('B', Bconv);	// big numbers
On 2011/10/26 22:28:36, rsc wrote:
> Move these into fmtinstallgo.

Done.

[rsc, 2011-10-28 14:41:26]

On Fri, Oct 28, 2011 at 04:02,  <lvd@google.com> wrote:
> because if you have type foo int63; type bar foo; this will print that
> back, otherwise you'd see type bar int63 for the second.
>
> this goes back to a bit of confusion between things and names for things
> in the design.  a similar thing happens for constants.

yes but that print is trying to show the underlying definition.

[lvd, 2011-10-28 17:30:05]

all tests pass, extra mode wasn't needed but we may revisit this later.  i
can still rename FSym (for generate-symbols-from-types, i htink i even
explained that in a comment.  still i'd like to get this in asap so as not
to pile up more merge pains.

/L

On Fri, Oct 28, 2011 at 16:41, Russ Cox <rsc@golang.org> wrote:

> On Fri, Oct 28, 2011 at 04:02,  <lvd@google.com> wrote:
> > because if you have type foo int63; type bar foo; this will print that
> > back, otherwise you'd see type bar int63 for the second.
> >
> > this goes back to a bit of confusion between things and names for things
> > in the design.  a similar thing happens for constants.
>
> yes but that print is trying to show the underlying definition.
>

restored the l.

[lvd, 2011-10-28 17:45:42]

On Fri, Oct 28, 2011 at 19:30, Luuk van Dijk <lvd@google.com> wrote:

> all tests pass, extra mode wasn't needed but we may revisit this later.  i
> can still rename FSym (for generate-symbols-from-types, i htink i even
> explained that in a comment.  still i'd like to get this in asap so as not
> to pile up more merge pains.


also not that the 'u' on %-uT no longer means half anothr mode but
specifically: qualify type names with pkg->prefix, and it's clearly
documented and all the mode knowledge is local to fmt.c, so i think the
current state is defensible.  hope you had a good flight.  i'm back online
tomorrow morning CET earliest, likely.


>
> /L
>
>
> On Fri, Oct 28, 2011 at 16:41, Russ Cox <rsc@golang.org> wrote:
>
>> On Fri, Oct 28, 2011 at 04:02,  <lvd@google.com> wrote:
>> > because if you have type foo int63; type bar foo; this will print that
>> > back, otherwise you'd see type bar int63 for the second.
>> >
>> > this goes back to a bit of confusion between things and names for things
>> > in the design.  a similar thing happens for constants.
>>
>> yes but that print is trying to show the underlying definition.
>>
>
> restored the l.
>

[rsc, 2011-10-31 15:04:45]

http://rsc.codereview.appspot.com/5316043/diff/4040/src/cmd/gc/fmt.c
File src/cmd/gc/fmt.c (right):

http://rsc.codereview.appspot.com/5316043/diff/4040/src/cmd/gc/fmt.c#newcode12
src/cmd/gc/fmt.c:12: //	%L  Line numbers
Please add the types here. 

%L int  line number
%O int  opcode
%J Node*
%V Val*
%S Sym*

etc

Also, please add %B, %E, %F, and %Z.

http://rsc.codereview.appspot.com/5316043/diff/4040/src/cmd/gc/fmt.c#newcode38
src/cmd/gc/fmt.c:38: FErr,	//     FmtSpace	 "% N" error mode (default)
can drop the FmtXxx names

http://rsc.codereview.appspot.com/5316043/diff/4040/src/cmd/gc/fmt.c#newcode41
src/cmd/gc/fmt.c:41: FSym,	//     FmtLeft	 "%-N" turning-types-into-symbols-mode
Can this be Ftypeid or Fidentical please?
"turning-types-into-symbols" is only one aspect.
What's important is that identical types format identically

Ftypeid,  // "%-T" identical types format identically

http://rsc.codereview.appspot.com/5316043/diff/4040/src/cmd/gc/reflect.c
File src/cmd/gc/reflect.c (right):

http://rsc.codereview.appspot.com/5316043/diff/4040/src/cmd/gc/reflect.c#newc...
src/cmd/gc/reflect.c:665: if (t == bytetype || t == runetype)
This should be unnecessary: the print format should do this.
If this is necessary, it means the print format is broken for
the cases where these types appear inside other types.
Also, this code is not in typesym above, so it seems likely
that this is a dreg and can just be deleted.

http://rsc.codereview.appspot.com/5316043/diff/4040/test/ddd1.go
File test/ddd1.go (right):

http://rsc.codereview.appspot.com/5316043/diff/4040/test/ddd1.go#newcode18
test/ddd1.go:18: _ = sum("hello")  // ERROR ".hello. .type ideal string. as type
int|incompatible"
I don't believe this should have changed.
That implies that something is semantically different about printing.

http://rsc.codereview.appspot.com/5316043/diff/4040/test/named1.go
File test/named1.go (right):

http://rsc.codereview.appspot.com/5316043/diff/4040/test/named1.go#newcode40
test/named1.go:40: asBool(1 != 2) // ERROR "cannot use.*type ideal bool.*as type
Bool"
Same.

[rsc, 2011-10-31 15:17:53]

I think I understand all the formatting changes,
and they look good.

It seems like the space flag is never used in the
source tree.  Can we drop that bit of complexity?

http://rsc.codereview.appspot.com/5316043/diff/4040/src/cmd/gc/fmt.c
File src/cmd/gc/fmt.c (right):

http://rsc.codereview.appspot.com/5316043/diff/4040/src/cmd/gc/fmt.c#newcode480
src/cmd/gc/fmt.c:480: if(pkglookup(s->pkg->name, nil)->npkg > 1)
It would be nice to preserve the old comment here.

		// This one is for the user.  If the package name
		// was used by multiple packages, give the full
		// import path to disambiguate.

[lvd, 2011-10-31 15:39:55]

all done and fixed the ninit logic in stmtfmt.

all tests pass

http://rsc.codereview.appspot.com/5316043/diff/4040/src/cmd/gc/fmt.c
File src/cmd/gc/fmt.c (right):

http://rsc.codereview.appspot.com/5316043/diff/4040/src/cmd/gc/fmt.c#newcode12
src/cmd/gc/fmt.c:12: //	%L  Line numbers
On 2011/10/31 15:04:45, rsc wrote:
> Please add the types here. 
> 
> %L int  line number
> %O int  opcode
> %J Node*
> %V Val*
> %S Sym*
> 
> etc
> 
> Also, please add %B, %E, %F, and %Z.

Done.

http://rsc.codereview.appspot.com/5316043/diff/4040/src/cmd/gc/fmt.c#newcode41
src/cmd/gc/fmt.c:41: FSym,	//     FmtLeft	 "%-N" turning-types-into-symbols-mode
On 2011/10/31 15:04:45, rsc wrote:
> Can this be Ftypeid or Fidentical please?
> "turning-types-into-symbols" is only one aspect.
> What's important is that identical types format identically
> 
> Ftypeid,  // "%-T" identical types format identically

Done.

http://rsc.codereview.appspot.com/5316043/diff/4040/src/cmd/gc/fmt.c#newcode41
src/cmd/gc/fmt.c:41: FSym,	//     FmtLeft	 "%-N" turning-types-into-symbols-mode
On 2011/10/31 15:04:45, rsc wrote:
> Can this be Ftypeid or Fidentical please?
> "turning-types-into-symbols" is only one aspect.
> What's important is that identical types format identically
> 
> Ftypeid,  // "%-T" identical types format identically

Done.

http://rsc.codereview.appspot.com/5316043/diff/4040/src/cmd/gc/fmt.c#newcode480
src/cmd/gc/fmt.c:480: if(pkglookup(s->pkg->name, nil)->npkg > 1)
On 2011/10/31 15:17:53, rsc wrote:
> It would be nice to preserve the old comment here.
> 
> 		// This one is for the user.  If the package name
> 		// was used by multiple packages, give the full
> 		// import path to disambiguate.
> 

Done.

http://rsc.codereview.appspot.com/5316043/diff/4040/src/cmd/gc/reflect.c
File src/cmd/gc/reflect.c (right):

http://rsc.codereview.appspot.com/5316043/diff/4040/src/cmd/gc/reflect.c#newc...
src/cmd/gc/reflect.c:665: if (t == bytetype || t == runetype)
On 2011/10/31 15:04:45, rsc wrote:
> This should be unnecessary: the print format should do this.
> If this is necessary, it means the print format is broken for
> the cases where these types appear inside other types.
> Also, this code is not in typesym above, so it seems likely
> that this is a dreg and can just be deleted.

yes, leftover from earlier iteration.  gone.

http://rsc.codereview.appspot.com/5316043/diff/4040/test/ddd1.go
File test/ddd1.go (right):

http://rsc.codereview.appspot.com/5316043/diff/4040/test/ddd1.go#newcode18
test/ddd1.go:18: _ = sum("hello")  // ERROR ".hello. .type ideal string. as type
int|incompatible"
On 2011/10/31 15:04:45, rsc wrote:
> I don't believe this should have changed.
> That implies that something is semantically different about printing.

the old code didnt print ideal, b/c that thing in Tpretty never triggered, which
led me t ask 'who wants to see this' in the ported-over line,  which led you to
comment 'i do in error messages', so now i fixed it and it does.

in the old code, ideal would be inserted for t == idealstring || idealbool
except that for those,  the block above would trigger, when the l flag wasnt
specified, which was i think always in the relevant messages.

anyway, up to you: do you want to see 'ideal' here or not.

http://rsc.codereview.appspot.com/5316043/diff/4040/test/named1.go
File test/named1.go (right):

http://rsc.codereview.appspot.com/5316043/diff/4040/test/named1.go#newcode40
test/named1.go:40: asBool(1 != 2) // ERROR "cannot use.*type ideal bool.*as type
Bool"
On 2011/10/31 15:04:45, rsc wrote:
> Same.

same

[lvd, 2011-10-31 15:46:51]

On 2011/10/31 15:39:55, lvd wrote:
> all done and fixed the ninit logic in stmtfmt.
> 

took out the FmtSpace logic and adapted comments

> all tests pass

still

[rsc, 2011-10-31 17:06:26]

LGTM!

[lvd, 2011-10-31 17:09:53]

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

gc: clean up printing.

Got rid of all the magic mystery globals. Now
for %N, %T, and %S, the flags +,- and # set a sticky
debug, sym and export mode, only visible in the new fmt.c.
Default is error mode. Handle h and l flags consistently with
the least side effects, so we can now change
things without worrying about unrelated things
breaking.

fixes issue 2361

R=rsc
CC=golang-dev
http://codereview.appspot.com/5316043