code review 5598054: go/printer, gofmt: don't print incorrect programs

go/printer, gofmt: don't print incorrect programs

Be careful when printing line comments with incorrect
position information. Maintain additional state
impliedSemi: when set, a comment containing a newline
would imply a semicolon and thus placement must be
delayed.

Precompute state information pertaining to the next
comment for faster checks (the printer is marginally
faster now despite additional checks for each comment).

No effect on existing src, misc sources.

Fixes issue 1505.

[gri, 2012-02-07 01:41:36]

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

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

[rsc, 2012-02-07 22:56:41]

Looks good except for the note around printer.go:881.

http://codereview.appspot.com/5598054/diff/19001/src/pkg/go/printer/printer.go
File src/pkg/go/printer/printer.go (right):

http://codereview.appspot.com/5598054/diff/19001/src/pkg/go/printer/printer.g...
src/pkg/go/printer/printer.go:109: if len(text) >= 2 { // should always be the
case, but be cautious
if len(text) >= 2 && (text[1] == '/' || strings.Contains(text, "\n")) {
    return true
}

http://codereview.appspot.com/5598054/diff/19001/src/pkg/go/printer/printer.g...
src/pkg/go/printer/printer.go:794: 
var impliedSemi bool  // value for p.impliedSemi after this arg

http://codereview.appspot.com/5598054/diff/19001/src/pkg/go/printer/printer.g...
src/pkg/go/printer/printer.go:819: p.impliedSemi = false
Why is this one p.impliedSemi but all the others are impliedSemi?
Is it a bug?  If not, comment?

http://codereview.appspot.com/5598054/diff/19001/src/pkg/go/printer/printer.g...
src/pkg/go/printer/printer.go:881: // intersperse extra newlines if present in
the source
It seems like this might still introduce newlines where they don't belong.
Should this be conditional on wroteNewline || !impliedSemi ?

[gri, 2012-02-07 23:13:16]

PTAL

http://codereview.appspot.com/5598054/diff/19001/src/pkg/go/printer/printer.go
File src/pkg/go/printer/printer.go (right):

http://codereview.appspot.com/5598054/diff/19001/src/pkg/go/printer/printer.g...
src/pkg/go/printer/printer.go:109: if len(text) >= 2 { // should always be the
case, but be cautious
On 2012/02/07 22:56:41, rsc wrote:
> if len(text) >= 2 && (text[1] == '/' || strings.Contains(text, "\n")) {
>     return true
> }

sure. In general, the strings.Contains is slower, but /*-style comments are not
very common.
Done.

http://codereview.appspot.com/5598054/diff/19001/src/pkg/go/printer/printer.g...
src/pkg/go/printer/printer.go:794: 
On 2012/02/07 22:56:41, rsc wrote:
> var impliedSemi bool  // value for p.impliedSemi after this arg

Done.

http://codereview.appspot.com/5598054/diff/19001/src/pkg/go/printer/printer.g...
src/pkg/go/printer/printer.go:819: p.impliedSemi = false
On 2012/02/07 22:56:41, rsc wrote:
> Why is this one p.impliedSemi but all the others are impliedSemi?
> Is it a bug?  If not, comment?
> 

Not a bug. Newlines introduced here effect the current state (before arg), as
these newlines are printed when interspersing comments. Added comment.

http://codereview.appspot.com/5598054/diff/19001/src/pkg/go/printer/printer.g...
src/pkg/go/printer/printer.go:881: // intersperse extra newlines if present in
the source
On 2012/02/07 22:56:41, rsc wrote:
> It seems like this might still introduce newlines where they don't belong.
> Should this be conditional on wroteNewline || !impliedSemi ?

Hm, probably. I've added an if and verified that it doesn't affect anything we
have.

[rsc, 2012-02-07 23:17:29]

LGTM

[gri, 2012-02-07 23:19:56]

*** Submitted as 0ff7fac59df1 ***

go/printer, gofmt: don't print incorrect programs

Be careful when printing line comments with incorrect
position information. Maintain additional state
impliedSemi: when set, a comment containing a newline
would imply a semicolon and thus placement must be
delayed.

Precompute state information pertaining to the next
comment for faster checks (the printer is marginally
faster now despite additional checks for each comment).

No effect on existing src, misc sources.

Fixes issue 1505.

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