Skip to content

Commit 542ea5a

Browse files
griesemergriesemer
committed
go/printer, gofmt: tuned table alignment for better results
The go/printer (and thus gofmt) uses a heuristic to determine whether to break alignment between elements of an expression list which is spread across multiple lines. The heuristic only kicked in if the entry sizes (character length) was above a certain threshold (20) and the ratio between the previous and current entry size was above a certain value (4). This heuristic worked reasonably most of the time, but also led to unfortunate breaks in many cases where a single entry was suddenly much smaller (or larger) then the previous one. The behavior of gofmt was sufficiently mysterious in some of these situations that many issues were filed against it. The simplest solution to address this problem is to remove the heuristic altogether and have a programmer introduce empty lines to force different alignments if it improves readability. The problem with that approach is that the places where it really matters, very long tables with many (hundreds, or more) entries, may be machine-generated and not "post-processed" by a human (e.g., unicode/utf8/tables.go). If a single one of those entries is overlong, the result would be that the alignment would force all comments or values in key:value pairs to be adjusted to that overlong value, making the table hard to read (e.g., that entry may not even be visible on screen and all other entries seem spaced out too wide). Instead, we opted for a slightly improved heuristic that behaves much better for "normal", human-written code. 1) The threshold is increased from 20 to 40. This disables the heuristic for many common cases yet even if the alignment is not "ideal", 40 is not that many characters per line with todays screens, making it very likely that the entire line remains "visible" in an editor. 2) Changed the heuristic to not simply look at the size ratio between current and previous line, but instead considering the geometric mean of the sizes of the previous (aligned) lines. This emphasizes the "overall picture" of the previous lines, rather than a single one (which might be an outlier). 3) Changed the ratio from 4 to 2.5. Now that we ignore sizes below 40, a ratio of 4 would mean that a new entry would have to be 4 times bigger (160) or smaller (10) before alignment would be broken. A ratio of 2.5 seems more sensible. Applied updated gofmt to all of src and misc. Also tested against several former issues that complained about this and verified that the output for the given examples is satisfactory (added respective test cases). Some of the files changed because they were not gofmt-ed in the first place. For #644. For #7335. For #10392. (and probably more related issues) Fixes #22852. Change-Id: I5e48b3d3b157a5cf2d649833b7297b33f43a6f6e
1 parent d7c7d88 commit 542ea5a

File tree

32 files changed

+2782
-2500
lines changed

32 files changed

+2782
-2500
lines changed

‎src/archive/tar/reader_test.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -378,9 +378,9 @@ func TestReader(t *testing.T) {
378378
"security.selinux": "unconfined_u:object_r:default_t:s0\x00",
379379
},
380380
PAXRecords: map[string]string{
381-
"mtime": "1386065770.449252304",
382-
"atime": "1389782991.41987522",
383-
"ctime": "1386065770.449252304",
381+
"mtime": "1386065770.449252304",
382+
"atime": "1389782991.41987522",
383+
"ctime": "1386065770.449252304",
384384
"SCHILY.xattr.security.selinux": "unconfined_u:object_r:default_t:s0\x00",
385385
},
386386
Format: FormatPAX,

‎src/cmd/doc/doc_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -115,7 +115,7 @@ var tests = []test{
115115
`var MultiLineVar = map\[struct{ ... }\]struct{ ... }{ ... }`, // Multi line variable.
116116
`func MultiLineFunc\(x interface{ ... }\) \(r struct{ ... }\)`, // Multi line function.
117117
`var LongLine = newLongLine\(("someArgument[1-4]", ){4}...\)`, // Long list of arguments.
118-
`type T1 = T2`, // Type alias
118+
`type T1 = T2`, // Type alias
119119
},
120120
[]string{
121121
`const internalConstant = 2`, // No internal constants.

‎src/cmd/go/testdata/src/dupload/p2/p2.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,3 @@
11
package p2
2+
23
import _ "dupload/vendor/p"

‎src/cmd/go/testdata/src/exclude/x_linux.go

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,3 @@
11
// +build windows
22

33
package x
4-

‎src/cmd/go/testdata/src/my.pkg/main/main.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
package main
2+
23
import "my.pkg"
4+
35
func main() {
46
println(pkg.Text)
57
}

‎src/cmd/go/testdata/src/vetcycle/p.go

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,12 @@
11
package p
22

3-
43
type (
5-
_ interface{ m(B1) }
4+
_ interface{ m(B1) }
65
A1 interface{ a(D1) }
76
B1 interface{ A1 }
8-
C1 interface{ B1 /* ERROR issue #18395 */ }
7+
C1 interface {
8+
B1 /* ERROR issue #18395 */
9+
}
910
D1 interface{ C1 }
1011
)
1112

‎src/cmd/internal/obj/arm64/a.out.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -911,4 +911,4 @@ const (
911911
ARNG_H
912912
ARNG_S
913913
ARNG_D
914-
)
914+
)

‎src/cmd/internal/obj/arm64/asm7.go

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1668,13 +1668,13 @@ func cmp(a int, b int) bool {
16681668

16691669
case C_PSOREG_4:
16701670
switch b {
1671-
case C_ZOREG, C_PSOREG_8:
1671+
case C_ZOREG, C_PSOREG_8:
16721672
return true
16731673
}
16741674

16751675
case C_PSOREG:
16761676
switch b {
1677-
case C_ZOREG, C_PSOREG_8, C_PSOREG_4:
1677+
case C_ZOREG, C_PSOREG_8, C_PSOREG_4:
16781678
return true
16791679
}
16801680

@@ -4014,7 +4014,7 @@ func (c *ctxt7) asmout(p *obj.Prog, o *Optab, out []uint32) {
40144014
rt := int((p.To.Reg) & 31)
40154015
r := int((p.Reg) & 31)
40164016

4017-
o1 |= ((Q&1) << 30) | ((size&3) << 22) | (uint32(rf&31) << 16) | (uint32(r&31) << 5) | uint32(rt&31)
4017+
o1 |= ((Q & 1) << 30) | ((size & 3) << 22) | (uint32(rf&31) << 16) | (uint32(r&31) << 5) | uint32(rt&31)
40184018

40194019
case 94: /* vext $imm4, Vm.<T>, Vn.<T>, Vd.<T> */
40204020
if p.From3Type() != obj.TYPE_REG {
@@ -4053,7 +4053,7 @@ func (c *ctxt7) asmout(p *obj.Prog, o *Optab, out []uint32) {
40534053
rt := int((p.To.Reg) & 31)
40544054
r := int((p.Reg) & 31)
40554055

4056-
o1 |= ((Q&1) << 30) | (uint32(r&31) << 16) | (uint32(index&15) << 11) | (uint32(rf&31) << 5) | uint32(rt&31)
4056+
o1 |= ((Q & 1) << 30) | (uint32(r&31) << 16) | (uint32(index&15) << 11) | (uint32(rf&31) << 5) | uint32(rt&31)
40574057

40584058
case 95: /* vushr $shift, Vn.<T>, Vd.<T> */
40594059
at := int((p.To.Reg >> 5) & 15)
@@ -4111,7 +4111,7 @@ func (c *ctxt7) asmout(p *obj.Prog, o *Optab, out []uint32) {
41114111
rt := int((p.To.Reg) & 31)
41124112
rf := int((p.Reg) & 31)
41134113

4114-
o1 |= ((Q&1) << 30) | (uint32(imm&127) << 16) | (uint32(rf&31) << 5) | uint32(rt&31)
4114+
o1 |= ((Q & 1) << 30) | (uint32(imm&127) << 16) | (uint32(rf&31) << 5) | uint32(rt&31)
41154115

41164116
case 96: /* vst1 Vt1.<T>[index], offset(Rn) */
41174117
af := int((p.From.Reg >> 5) & 15)
@@ -4183,7 +4183,7 @@ func (c *ctxt7) asmout(p *obj.Prog, o *Optab, out []uint32) {
41834183
o1 |= 26 << 23
41844184
}
41854185

4186-
o1 |= (uint32(Q&1) << 30) | (uint32(r&31) << 16) | ((opcode&7) << 13) | (uint32(S&1) << 12) | (uint32(size&3) << 10) | (uint32(rf&31) << 5) | uint32(rt&31)
4186+
o1 |= (uint32(Q&1) << 30) | (uint32(r&31) << 16) | ((opcode & 7) << 13) | (uint32(S&1) << 12) | (uint32(size&3) << 10) | (uint32(rf&31) << 5) | uint32(rt&31)
41874187

41884188
case 97: /* vld1 offset(Rn), vt.<T>[index] */
41894189
at := int((p.To.Reg >> 5) & 15)
@@ -4257,7 +4257,7 @@ func (c *ctxt7) asmout(p *obj.Prog, o *Optab, out []uint32) {
42574257
o1 |= 106 << 21
42584258
}
42594259

4260-
o1 |= (uint32(Q&1) << 30) | (uint32(r&31) << 16) | ((opcode&7) << 13) | (uint32(S&1) << 12) | (uint32(size&3) << 10) | (uint32(rf&31) << 5) | uint32(rt&31)
4260+
o1 |= (uint32(Q&1) << 30) | (uint32(r&31) << 16) | ((opcode & 7) << 13) | (uint32(S&1) << 12) | (uint32(size&3) << 10) | (uint32(rf&31) << 5) | uint32(rt&31)
42614261
}
42624262
out[0] = o1
42634263
out[1] = o2

‎src/cmd/pprof/pprof.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -75,8 +75,8 @@ func getProfile(source string, timeout time.Duration) (*profile.Profile, error)
7575
client := &http.Client{
7676
Transport: &http.Transport{
7777
ResponseHeaderTimeout: timeout + 5*time.Second,
78-
Proxy: http.ProxyFromEnvironment,
79-
TLSClientConfig: tlsConfig,
78+
Proxy: http.ProxyFromEnvironment,
79+
TLSClientConfig: tlsConfig,
8080
},
8181
}
8282
resp, err := client.Get(source)

‎src/cmd/vendor/github.com/google/pprof/internal/driver/fetch.go

Lines changed: 2 additions & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎src/cmd/vet/testdata/composite.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -115,6 +115,6 @@ var goodNamedPointerSliceLiteral = []*unicode.CaseRange{
115115
&unicode.CaseRange{Lo: 1, Hi: 2},
116116
}
117117
var badNamedPointerSliceLiteral = []*unicode.CaseRange{
118-
{1, 2}, // ERROR "unkeyed fields"
118+
{1, 2}, // ERROR "unkeyed fields"
119119
&unicode.CaseRange{1, 2}, // ERROR "unkeyed fields"
120120
}

‎src/crypto/tls/handshake_server_test.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -194,9 +194,9 @@ func TestDontSelectRSAWithECDSAKey(t *testing.T) {
194194

195195
func TestRenegotiationExtension(t *testing.T) {
196196
clientHello := &clientHelloMsg{
197-
vers: VersionTLS12,
198-
compressionMethods: []uint8{compressionNone},
199-
random: make([]byte, 32),
197+
vers: VersionTLS12,
198+
compressionMethods: []uint8{compressionNone},
199+
random: make([]byte, 32),
200200
secureRenegotiationSupported: true,
201201
cipherSuites: []uint16{TLS_RSA_WITH_RC4_128_SHA},
202202
}
@@ -992,7 +992,7 @@ func TestFallbackSCSV(t *testing.T) {
992992
name: "FallbackSCSV",
993993
config: &serverConfig,
994994
// OpenSSL 1.0.1j is needed for the -fallback_scsv option.
995-
command: []string{"openssl", "s_client", "-fallback_scsv"},
995+
command: []string{"openssl", "s_client", "-fallback_scsv"},
996996
expectHandshakeErrorIncluding: "inappropriate protocol fallback",
997997
}
998998
runServerTestTLS11(t, test)

‎src/crypto/x509/name_constraints_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1573,7 +1573,7 @@ func makeConstraintsCACert(constraints constraintsSpec, name string, key *ecdsa.
15731573
NotAfter: time.Unix(2000, 0),
15741574
KeyUsage: KeyUsageCertSign,
15751575
BasicConstraintsValid: true,
1576-
IsCA: true,
1576+
IsCA: true,
15771577
}
15781578

15791579
if err := addConstraintsToTemplate(constraints, template); err != nil {
@@ -1611,7 +1611,7 @@ func makeConstraintsLeafCert(leaf leafSpec, key *ecdsa.PrivateKey, parent *Certi
16111611
NotAfter: time.Unix(2000, 0),
16121612
KeyUsage: KeyUsageDigitalSignature,
16131613
BasicConstraintsValid: true,
1614-
IsCA: false,
1614+
IsCA: false,
16151615
}
16161616

16171617
for _, name := range leaf.sans {

‎src/crypto/x509/x509.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2556,7 +2556,7 @@ func ParseCertificateRequest(asn1Data []byte) (*CertificateRequest, error) {
25562556

25572557
func parseCertificateRequest(in *certificateRequest) (*CertificateRequest, error) {
25582558
out := &CertificateRequest{
2559-
Raw: in.Raw,
2559+
Raw: in.Raw,
25602560
RawTBSCertificateRequest: in.TBSCSR.Raw,
25612561
RawSubjectPublicKeyInfo: in.TBSCSR.PublicKey.Raw,
25622562
RawSubject: in.TBSCSR.Subject.FullBytes,

‎src/crypto/x509/x509_test.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -551,7 +551,7 @@ func TestCreateSelfSignedCertificate(t *testing.T) {
551551
UnknownExtKeyUsage: testUnknownExtKeyUsage,
552552

553553
BasicConstraintsValid: true,
554-
IsCA: true,
554+
IsCA: true,
555555

556556
OCSPServer: []string{"http://ocsp.example.com"},
557557
IssuingCertificateURL: []string{"http://crt.example.com/ca1.crt"},
@@ -1399,7 +1399,7 @@ func TestMaxPathLen(t *testing.T) {
13991399
NotAfter: time.Unix(100000, 0),
14001400

14011401
BasicConstraintsValid: true,
1402-
IsCA: true,
1402+
IsCA: true,
14031403
}
14041404

14051405
cert1 := serialiseAndParse(t, template)
@@ -1440,8 +1440,8 @@ func TestNoAuthorityKeyIdInSelfSignedCert(t *testing.T) {
14401440
NotAfter: time.Unix(100000, 0),
14411441

14421442
BasicConstraintsValid: true,
1443-
IsCA: true,
1444-
SubjectKeyId: []byte{1, 2, 3, 4},
1443+
IsCA: true,
1444+
SubjectKeyId: []byte{1, 2, 3, 4},
14451445
}
14461446

14471447
if cert := serialiseAndParse(t, template); len(cert.AuthorityKeyId) != 0 {

‎src/encoding/json/decode_test.go

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -615,9 +615,9 @@ var unmarshalTests = []unmarshalTest{
615615
out: S5{S8: S8{S9: S9{Y: 2}}},
616616
},
617617
{
618-
in: `{"X": 1,"Y":2}`,
619-
ptr: new(S5),
620-
err: fmt.Errorf("json: unknown field \"X\""),
618+
in: `{"X": 1,"Y":2}`,
619+
ptr: new(S5),
620+
err: fmt.Errorf("json: unknown field \"X\""),
621621
disallowUnknownFields: true,
622622
},
623623
{
@@ -626,9 +626,9 @@ var unmarshalTests = []unmarshalTest{
626626
out: S10{S13: S13{S8: S8{S9: S9{Y: 2}}}},
627627
},
628628
{
629-
in: `{"X": 1,"Y":2}`,
630-
ptr: new(S10),
631-
err: fmt.Errorf("json: unknown field \"X\""),
629+
in: `{"X": 1,"Y":2}`,
630+
ptr: new(S10),
631+
err: fmt.Errorf("json: unknown field \"X\""),
632632
disallowUnknownFields: true,
633633
},
634634

@@ -835,8 +835,8 @@ var unmarshalTests = []unmarshalTest{
835835
"Q": 18,
836836
"extra": true
837837
}`,
838-
ptr: new(Top),
839-
err: fmt.Errorf("json: unknown field \"extra\""),
838+
ptr: new(Top),
839+
err: fmt.Errorf("json: unknown field \"extra\""),
840840
disallowUnknownFields: true,
841841
},
842842
{
@@ -862,8 +862,8 @@ var unmarshalTests = []unmarshalTest{
862862
"Z": 17,
863863
"Q": 18
864864
}`,
865-
ptr: new(Top),
866-
err: fmt.Errorf("json: unknown field \"extra\""),
865+
ptr: new(Top),
866+
err: fmt.Errorf("json: unknown field \"extra\""),
867867
disallowUnknownFields: true,
868868
},
869869
}

‎src/go/build/deps_test.go

Lines changed: 43 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -229,49 +229,49 @@ var pkgDeps = map[string][]string{
229229
"go/types": {"L4", "GOPARSER", "container/heap", "go/constant"},
230230

231231
// One of a kind.
232-
"archive/tar": {"L4", "OS", "syscall", "os/user"},
233-
"archive/zip": {"L4", "OS", "compress/flate"},
234-
"container/heap": {"sort"},
235-
"compress/bzip2": {"L4"},
236-
"compress/flate": {"L4"},
237-
"compress/gzip": {"L4", "compress/flate"},
238-
"compress/lzw": {"L4"},
239-
"compress/zlib": {"L4", "compress/flate"},
240-
"context": {"errors", "fmt", "reflect", "sync", "time"},
241-
"database/sql": {"L4", "container/list", "context", "database/sql/driver", "database/sql/internal"},
242-
"database/sql/driver": {"L4", "context", "time", "database/sql/internal"},
243-
"debug/dwarf": {"L4"},
244-
"debug/elf": {"L4", "OS", "debug/dwarf", "compress/zlib"},
245-
"debug/gosym": {"L4"},
246-
"debug/macho": {"L4", "OS", "debug/dwarf"},
247-
"debug/pe": {"L4", "OS", "debug/dwarf"},
248-
"debug/plan9obj": {"L4", "OS"},
249-
"encoding": {"L4"},
250-
"encoding/ascii85": {"L4"},
251-
"encoding/asn1": {"L4", "math/big"},
252-
"encoding/csv": {"L4"},
253-
"encoding/gob": {"L4", "OS", "encoding"},
254-
"encoding/hex": {"L4"},
255-
"encoding/json": {"L4", "encoding"},
256-
"encoding/pem": {"L4"},
257-
"encoding/xml": {"L4", "encoding"},
258-
"flag": {"L4", "OS"},
259-
"go/build": {"L4", "OS", "GOPARSER"},
260-
"html": {"L4"},
261-
"image/draw": {"L4", "image/internal/imageutil"},
262-
"image/gif": {"L4", "compress/lzw", "image/color/palette", "image/draw"},
263-
"image/internal/imageutil": {"L4"},
264-
"image/jpeg": {"L4", "image/internal/imageutil"},
265-
"image/png": {"L4", "compress/zlib"},
266-
"index/suffixarray": {"L4", "regexp"},
267-
"internal/singleflight": {"sync"},
268-
"internal/trace": {"L4", "OS"},
269-
"math/big": {"L4"},
270-
"mime": {"L4", "OS", "syscall", "internal/syscall/windows/registry"},
271-
"mime/quotedprintable": {"L4"},
272-
"net/internal/socktest": {"L4", "OS", "syscall", "internal/syscall/windows"},
273-
"net/url": {"L4"},
274-
"plugin": {"L0", "OS", "CGO"},
232+
"archive/tar": {"L4", "OS", "syscall", "os/user"},
233+
"archive/zip": {"L4", "OS", "compress/flate"},
234+
"container/heap": {"sort"},
235+
"compress/bzip2": {"L4"},
236+
"compress/flate": {"L4"},
237+
"compress/gzip": {"L4", "compress/flate"},
238+
"compress/lzw": {"L4"},
239+
"compress/zlib": {"L4", "compress/flate"},
240+
"context": {"errors", "fmt", "reflect", "sync", "time"},
241+
"database/sql": {"L4", "container/list", "context", "database/sql/driver", "database/sql/internal"},
242+
"database/sql/driver": {"L4", "context", "time", "database/sql/internal"},
243+
"debug/dwarf": {"L4"},
244+
"debug/elf": {"L4", "OS", "debug/dwarf", "compress/zlib"},
245+
"debug/gosym": {"L4"},
246+
"debug/macho": {"L4", "OS", "debug/dwarf"},
247+
"debug/pe": {"L4", "OS", "debug/dwarf"},
248+
"debug/plan9obj": {"L4", "OS"},
249+
"encoding": {"L4"},
250+
"encoding/ascii85": {"L4"},
251+
"encoding/asn1": {"L4", "math/big"},
252+
"encoding/csv": {"L4"},
253+
"encoding/gob": {"L4", "OS", "encoding"},
254+
"encoding/hex": {"L4"},
255+
"encoding/json": {"L4", "encoding"},
256+
"encoding/pem": {"L4"},
257+
"encoding/xml": {"L4", "encoding"},
258+
"flag": {"L4", "OS"},
259+
"go/build": {"L4", "OS", "GOPARSER"},
260+
"html": {"L4"},
261+
"image/draw": {"L4", "image/internal/imageutil"},
262+
"image/gif": {"L4", "compress/lzw", "image/color/palette", "image/draw"},
263+
"image/internal/imageutil": {"L4"},
264+
"image/jpeg": {"L4", "image/internal/imageutil"},
265+
"image/png": {"L4", "compress/zlib"},
266+
"index/suffixarray": {"L4", "regexp"},
267+
"internal/singleflight": {"sync"},
268+
"internal/trace": {"L4", "OS"},
269+
"math/big": {"L4"},
270+
"mime": {"L4", "OS", "syscall", "internal/syscall/windows/registry"},
271+
"mime/quotedprintable": {"L4"},
272+
"net/internal/socktest": {"L4", "OS", "syscall", "internal/syscall/windows"},
273+
"net/url": {"L4"},
274+
"plugin": {"L0", "OS", "CGO"},
275275
"runtime/pprof/internal/profile": {"L4", "OS", "compress/gzip", "regexp"},
276276
"testing/internal/testdeps": {"L4", "internal/testlog", "runtime/pprof", "regexp"},
277277
"text/scanner": {"L4", "OS"},

0 commit comments

Comments
 (0)