code review 6294076: gofmt: handle comments correctly in rewrites

gofmt: handle comments correctly in rewrites

[gri, 2012-06-15 21:09:57]

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-06-25 14:12:49]

Is this really correct? I understand the rationale, but in the example test data
I am not sure that losing the comment is really something that an automated tool
should be doing.

[gri, 2012-06-25 18:27:06]

It is actually not very easy to create a test case that loses
comments. Most of the time the comment is associated with a larger
construct (such as a statement, declaration, etc.) so the comment is
not lost unless the entire statement is lost. In this case, the
comment is associated with a single expression (deliberately
constructed), and that expression goes away. It seems only right that
the comment goes away.

Furthermore, when applying gofmt -r or -s one is very likely going to
inspect the result, so I don't think there's much danger here.

Keeping the comment seems more wrong in this case.

- Robert

On Mon, Jun 25, 2012 at 7:12 AM,  <rsc@golang.org> wrote:
> Is this really correct? I understand the rationale, but in the example
> test data I am not sure that losing the comment is really something that
> an automated tool should be doing.
>
>
>
> http://codereview.appspot.com/6294076/

[rsc, 2012-06-25 18:30:54]

The problem with deleting a comment is that you might not, on
inspection, notice that anything is missing. I don't mind gofmt -r
doing this, but having gofmt -s do it seems wrong. The kinds of
rewrites that gofmt -s does are supposed to be so trivial that you can
run it automatically like plain gofmt; if gofmt -s can delete comments
then we'd have to be much more careful using it.

Russ

[gri, 2012-06-25 18:41:09]

It's easy to restrict it -r. But again, it's difficult to create a
case that deletes comments with -s: it will only happen if the comment
is directly associated with something that goes away. For instance in:

package p

func f() {
	var a []string
	for i /*foo*/, _ /*bar*/ := range a /*bal*/ {
		_ = i
	}
}

gofmt -s produces:

package p

func f() {
	var a []string
	for i /*foo*/ := range a /*bal*/ {
		_ = i
	}
}

which seems right.

Also, each -s optimization is programmed explicitly, and if we care
about things going away that might have important comments, the code
should be adjusted to take care of that comment (via
commentMap.Update).

?

- Robert



On Mon, Jun 25, 2012 at 11:30 AM, Russ Cox <rsc@golang.org> wrote:
> The problem with deleting a comment is that you might not, on
> inspection, notice that anything is missing. I don't mind gofmt -r
> doing this, but having gofmt -s do it seems wrong. The kinds of
> rewrites that gofmt -s does are supposed to be so trivial that you can
> run it automatically like plain gofmt; if gofmt -s can delete comments
> then we'd have to be much more careful using it.
>
> Russ

[rsc, 2012-06-25 18:44:54]

Please leave the comments in for -s.

The fact that it is so hard to create a situation where there is a
difference means that when a difference does come up (and a comment is
deleted) people will be surprised. I am trying to avoid that surprise:
gofmt -s should be as reliable about not deleting parts of your
program as plain gofmt is. Instead of having to program to keep the
comments explicitly, we can just not use the comment map to delete
comments. Then we know that all comments are preserved, automatically,
which seems much less error-prone.

gofmt -r is doing arbitrary rewriting, so I don't mind dropping comments there.

Russ

[gri, 2012-06-25 20:54:29]

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

Please take another look.

[rsc, 2012-06-25 20:56:15]

LGTM

[gri, 2012-06-25 20:58:35]

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

gofmt: handle comments correctly in rewrites

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