Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/gofmt: comment lines broken at new position #18782

Closed
kortschak opened this issue Jan 25, 2017 · 11 comments
Closed

cmd/gofmt: comment lines broken at new position #18782

kortschak opened this issue Jan 25, 2017 · 11 comments
Assignees
Milestone

Comments

@kortschak
Copy link
Contributor

@kortschak kortschak commented Jan 25, 2017

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

go1.8rc2

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOOS="linux"

What did you do?

If possible, provide a recipe for reproducing the error.
A complete runnable program is good.
A link on play.golang.org is best.
Run gofmt -d on https://github.com/biogo/biogo/blob/master/align/matrix/matrices.go

What did you expect to see?

No difference.

What did you see instead?

Log output here https://travis-ci.org/biogo/biogo/jobs/195029064#L501

@griesemer
Copy link
Contributor

@griesemer griesemer commented Jan 25, 2017

This appears to be a regression from 1.7. The problem doesn't appear with https://play.golang.org/p/iptkp861qM (simple repro case) in the playground. But with 1.8 we get:

package p

var _ = [][]int{
	/*       a, b, c, d, e */
	/* a */
	{0, 0, 0, 0, 0},
	/* b */ {0, 5, 4, 4, 4},
	/* c */ {0, 4, 5, 4, 4},
	/* d */ {0, 4, 4, 5, 4},
	/* e */ {0, 4, 4, 4, 5},
}

Likely related to https://go-review.googlesource.com/#/c/33016/ (need to confirm).

It's probably too late to get this fixed for 1.8 unless there's a trivial mistake. Unrolling the previous change (if it's the culprit) would expose two other issues that it fixed.

@griesemer griesemer added this to the Go1.9 milestone Jan 25, 2017
@griesemer griesemer added the NeedsFix label Jan 25, 2017
@griesemer griesemer modified the milestones: Go1.9Early, Go1.9 Jan 25, 2017
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Jan 25, 2017

@griesemer, you're confident this is sufficiently rare to push off to Go 1.9?

@griesemer
Copy link
Contributor

@griesemer griesemer commented Jan 25, 2017

@bradfitz I haven't seen this before and none of the code we regularly gofmt appears to have the problem. That is all I can say at the moment. I will look into the problem today - perhaps there's a safe and easy fix.

@griesemer
Copy link
Contributor

@griesemer griesemer commented Jan 25, 2017

Confirming that https://go-review.googlesource.com/#/c/33016/ (commit a0d2e96) is indeed the culprit. The bug is not present before (commit 8cd5561).

@kortschak
Copy link
Contributor Author

@kortschak kortschak commented Jan 25, 2017

@griesemer, you're confident this is sufficiently rare to push off to Go 1.9?

As a data point, this breaks my CI as I perform automated gomft checks on that repo. The failure masks other real failures.

@griesemer
Copy link
Contributor

@griesemer griesemer commented Jan 25, 2017

@kortschak Understood. I've started working on this.

@kortschak
Copy link
Contributor Author

@kortschak kortschak commented Jan 25, 2017

@griesemer Thank you

@griesemer
Copy link
Contributor

@griesemer griesemer commented Jan 25, 2017

@kortschak Not a completely satisfactory "solution", but a work-around: The following source

var _ = [][]int{ /* a, b, c, d, e */
	/*    a */ {0, 0, 0, 0, 0},
	/*    b */ {0, 5, 4, 4, 4},
	/*    c */ {0, 4, 5, 4, 4},
	/*    d */ {0, 4, 4, 5, 4},
	/*    e */ {0, 4, 4, 4, 5},
}

(first comment on same line as /*) will be preserved. Still trying to see if there's a simple (1.8-acceptable solution).

@griesemer
Copy link
Contributor

@griesemer griesemer commented Jan 25, 2017

@kortschak Can you verify that https://go-review.googlesource.com/35811 fixes the problem for you? Thanks.

@gopherbot
Copy link

@gopherbot gopherbot commented Jan 25, 2017

CL https://golang.org/cl/35811 mentions this issue.

@kortschak
Copy link
Contributor Author

@kortschak kortschak commented Jan 25, 2017

@griesemer Yes, that fixes the problem. Thanks.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.