code review 174051: New flags for gofmt:

New flags for gofmt:
- oldparser            parse old syntax (required semicolons)
- oldprinter           print old syntax (required semicolons)

By default, these flags are enabled for now.
Setting -oldparser=false has no effect until go/parser is changed
to accept the new syntax.

Enabled exp/parser in Makefile; update dependent exp/eval.

[gri, 2009-12-10 23:39:13]

Hello rsc,

I'd like you to review the following change.

[rsc, 2009-12-10 23:53:33]

LGTM

http://codereview.appspot.com/174051/diff/1002/1003
File src/cmd/gofmt/gofmt.go (right):

http://codereview.appspot.com/174051/diff/1002/1003#newcode39
src/cmd/gofmt/gofmt.go:39: insertSemis		= flag.Bool("insertsemis", false,
"accept automatically inserted semicolons");
It's not clear to me (just looking at the name) whether
the semis are inserted on input or output.  printNoSemis
seems ambiguous in a different way, since printNoSemis=false
still doesn't print as many semicolons as possible.

Maybe these flags should be

useOldParser = flag.Bool("oldparser", true, "parse old (required semicolons)
syntax");
useOldPrinter = flag.Bool("oldprinter", true, "print old (required semicolons)
syntax");

Can printNoStringConcat be dropped entirely?  Now that the tree
doesn't use string concatenation anymore, it seems okay for the
default (and only) behavior to be to insert the +.

[gri, 2009-12-11 00:12:24]

PTAL


On Thu, Dec 10, 2009 at 3:53 PM, <rsc@golang.org> wrote:

> LGTM
>
>
>
> http://codereview.appspot.com/174051/diff/1002/1003
> File src/cmd/gofmt/gofmt.go (right):
>
> http://codereview.appspot.com/174051/diff/1002/1003#newcode39
> src/cmd/gofmt/gofmt.go:39: insertSemis          = flag.Bool("insertsemis",
>
> false, "accept automatically inserted semicolons");
> It's not clear to me (just looking at the name) whether
> the semis are inserted on input or output.  printNoSemis
> seems ambiguous in a different way, since printNoSemis=false
> still doesn't print as many semicolons as possible.
>
> Maybe these flags should be
>
> useOldParser = flag.Bool("oldparser", true, "parse old (required
> semicolons) syntax");
> useOldPrinter = flag.Bool("oldprinter", true, "print old (required
> semicolons) syntax");
>
> Can printNoStringConcat be dropped entirely?  Now that the tree
> doesn't use string concatenation anymore, it seems okay for the
> default (and only) behavior to be to insert the +.
>
>
> http://codereview.appspot.com/174051
>

[gri, 2009-12-11 00:22:29]

I forgot to say that I think for now we should keep the insertstringplus
flag since we don't know how many people out there would like to convert
their existing code.
- Robert

On Thu, Dec 10, 2009 at 4:12 PM, Robert Griesemer <gri@golang.org> wrote:

> PTAL
>
>
> On Thu, Dec 10, 2009 at 3:53 PM, <rsc@golang.org> wrote:
>
>> LGTM
>>
>>
>>
>> http://codereview.appspot.com/174051/diff/1002/1003
>> File src/cmd/gofmt/gofmt.go (right):
>>
>> http://codereview.appspot.com/174051/diff/1002/1003#newcode39
>> src/cmd/gofmt/gofmt.go:39: insertSemis          = flag.Bool("insertsemis",
>>
>> false, "accept automatically inserted semicolons");
>> It's not clear to me (just looking at the name) whether
>> the semis are inserted on input or output.  printNoSemis
>> seems ambiguous in a different way, since printNoSemis=false
>> still doesn't print as many semicolons as possible.
>>
>> Maybe these flags should be
>>
>> useOldParser = flag.Bool("oldparser", true, "parse old (required
>> semicolons) syntax");
>> useOldPrinter = flag.Bool("oldprinter", true, "print old (required
>> semicolons) syntax");
>>
>> Can printNoStringConcat be dropped entirely?  Now that the tree
>> doesn't use string concatenation anymore, it seems okay for the
>> default (and only) behavior to be to insert the +.
>>
>>
>> http://codereview.appspot.com/174051
>>
>
>

[rsc, 2009-12-11 02:12:24]

LGTM

check all.bash - not sure whether exp/parser
should be added to the NOTEST list.

[gri, 2009-12-11 03:03:31]

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

New flags for gofmt:
- oldparser            parse old syntax (required semicolons)
- oldprinter           print old syntax (required semicolons)

By default, these flags are enabled for now.
Setting -oldparser=false has no effect until go/parser is changed
to accept the new syntax.

Enabled exp/parser in Makefile; update dependent exp/eval.

R=rsc
http://codereview.appspot.com/174051