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: inconsistent spacing on slice indices #11497

Open
tgrosinger opened this issue Jul 1, 2015 · 4 comments
Open

cmd/gofmt: inconsistent spacing on slice indices #11497

tgrosinger opened this issue Jul 1, 2015 · 4 comments
Assignees
Milestone

Comments

@tgrosinger
Copy link

@tgrosinger tgrosinger commented Jul 1, 2015

What version of Go are you using (go version)? go version go1.4.2 linux/amd64
What operating system and processor architecture are you using? Ubuntu 14.04
What did you do? Run gofmt on below code examples
What did you expect to see? Consistent spacing of slice indices

These are two lines within the same function. They vary slightly, but the interesting part is the [1:len(key-1] which has inconsistent spacing. Adding spaces to the first one will result in them being removed when gofmt is run, and removing the spaces from the second will only have them added back.

If this is not a bug, please let me know what difference here is causing the spacing to change.

return "?" + key[1:len(key)-1]
key = key[1 : len(key)-1]
@adg
Copy link
Contributor

@adg adg commented Jul 1, 2015

It's working as intended. There's a good explanation here.

We like regularity too, but we like readability more,
and the few breaks from regularity in gofmt are for
things that we felt, after over a year of writing
Go programs, enhanced readability enough to justify
breaking from the simpler rules.

In this specific case, 1<<x + y is much clearer as
to its meaning than 1 << x + y, because it emphasizes
the precedence.

Your example - "x && rand.Int()%RAND_FACT" - is not
the whole story: you lifted a fragment. The real expression
would have been something like

x && rand.Int()%RAND_FACT != 0 

since a % cannot produce a bool and && requires one.
If the op were & instead of %, like in

x && rand.Int()&RAND_MASK != 0 

then I appreciate the visual reinforcement that it's being
parsed the way I intend (i.e. it has a different meaning
here than it would in C).

Especially because operator precedence is a notorious
source of confusion in C and because Go changed
the precedences to simplify things, we think this is an
important way to help programmers adjust.

@adg adg closed this Jul 1, 2015
@mikioh mikioh changed the title GoFmt inconsistent spacing on slice indices cmd/gofmt: inconsistent spacing on slice indices Jul 2, 2015
@tgrosinger
Copy link
Author

@tgrosinger tgrosinger commented Jul 2, 2015

Hmm, I understand that there may be times where one formatting is better than another. But in this case the core piece of the line is exactly the same in both cases. Is there a more specific reasoning that causes this particular difference?

@griesemer
Copy link
Contributor

@griesemer griesemer commented Jul 2, 2015

@tgrosinger You are correct - they should format the same:

  1. The spacing changes when going from http://play.golang.org/p/ToSuqHfqWu to http://play.golang.org/p/rVyw-A9om1 - the difference is just the added parentheses around the slice expression. However, the slice expression already introduces a form of parentheses (square brackets), so inside there shouldn't be a change.

  2. There are long-standing TODOs for exactly this issue in go/printer:776 (also line 769).

The fix is trivial, but in the interest of not introducing lots of changes over huge amounts of code, we haven't pushed it through, and I suggest leaving it as is for now.

That said, I've been working on a new version of gofmt (long overdue, hopefully after 1.5 I can concentrate on finishing it) which should solve many of the open gofmt issues. This may be a good time to address this one as well.

@griesemer griesemer reopened this Jul 2, 2015
@griesemer griesemer self-assigned this Jul 2, 2015
@griesemer griesemer added this to the Unplanned milestone Jul 2, 2015
@tgrosinger
Copy link
Author

@tgrosinger tgrosinger commented Jul 2, 2015

Good to know, thank you.

bzzoop added a commit to cactus/gostrftime that referenced this issue Dec 16, 2015
gofmt
gofmt formats this file differently than it used to, and results in what
at first seems like an odd inconsistency. Apparently this is by design
(see [this][1] and [this][2]), and due to nesting level.

[1]: golang/go#11497
[2]: golang/go#12720
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

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