Laszlo Ersek
2023-Jul-27 11:52 UTC
[Libguestfs] [libnbd PATCH 5/7] golang: Use 'gofmt' style recommendations on manual files
On 7/26/23 19:50, Eric Blake wrote:> I ran: > gofmt -s -w $(git ls-files -- '**/*.go') > > then touched up a few comments in test 590 where it mis-interpreted > our intentions of having a single sentence that occupies more than 80 > columns. > > Most of the changes are whitespace fixes (consistent use of TAB, > altering the whitespace around operators), using preferred ordering > for imports, and eliding excess syntax (unneeded () or ;). > > This patch only adjusts files directly in git control. Later patches > will tackle (most) Go style problems in the generated files, then > automate a way to ensure we don't regress. > > Signed-off-by: Eric Blake <eblake at redhat.com> > --- > golang/configure/test.go | 14 ++++---- > golang/examples/aio_copy/aio_copy.go | 7 ++-- > golang/examples/simple_copy/simple_copy.go | 3 +- > golang/libnbd_220_opt_list_test.go | 4 +-- > golang/libnbd_240_opt_list_meta_test.go | 32 +++++++++---------- > .../libnbd_245_opt_list_meta_queries_test.go | 14 ++++---- > golang/libnbd_250_opt_set_meta_test.go | 2 +- > .../libnbd_255_opt_set_meta_queries_test.go | 4 +-- > golang/libnbd_405_pread_structured_test.go | 10 +++--- > golang/libnbd_590_aio_copy_test.go | 21 ++++++++---- > golang/libnbd_620_stats_test.go | 12 +++---- > 11 files changed, 65 insertions(+), 58 deletions(-) > > diff --git a/golang/configure/test.go b/golang/configure/test.go > index 11911377..fe742f2b 100644 > --- a/golang/configure/test.go > +++ b/golang/configure/test.go > @@ -31,11 +31,11 @@ func main() { > fmt.Println(runtime.Version()) > > /* XXX Check for minimum runtime.Version() >= "go1.1.1" > - * Unfortunately go version numbers are not easy to parse. > - * They have the 3 formats "goX.Y.Z", "release.rN" or > - * "weekly.YYYY-MM-DD". The latter two formats are mostly > - * useless, and the first one is hard to parse. See also > - * cmpGoVersion in > - * http://web.archive.org/web/20130402235148/http://golang.org/src/cmd/go/get.go?m=text > - */ > + * Unfortunately go version numbers are not easy to parse. > + * They have the 3 formats "goX.Y.Z", "release.rN" or > + * "weekly.YYYY-MM-DD". The latter two formats are mostly > + * useless, and the first one is hard to parse. See also > + * cmpGoVersion in > + * http://web.archive.org/web/20130402235148/http://golang.org/src/cmd/go/get.go?m=text > + */ > } > diff --git a/golang/examples/aio_copy/aio_copy.go b/golang/examples/aio_copy/aio_copy.go > index e3093da8..1de115b1 100644 > --- a/golang/examples/aio_copy/aio_copy.go > +++ b/golang/examples/aio_copy/aio_copy.go > @@ -36,8 +36,7 @@ > // > // Example: > // > -// ./aio_copy nbd+unix:///?socket=/tmp.nbd >/dev/null > -// > +// ./aio_copy nbd+unix:///?socket=/tmp.nbd >/dev/null > package main > > import ( > @@ -66,8 +65,8 @@ > // libnbd, until the command reach the front of the queue and can be writen to > // the output. > type command struct { > - buf libnbd.AioBuffer > - ready bool > + buf libnbd.AioBuffer > + ready bool > } > > func main() { > diff --git a/golang/examples/simple_copy/simple_copy.go b/golang/examples/simple_copy/simple_copy.go > index aa9e89b4..7c4182f6 100644 > --- a/golang/examples/simple_copy/simple_copy.go > +++ b/golang/examples/simple_copy/simple_copy.go > @@ -36,8 +36,7 @@ > // > // Example: > // > -// ./simple_copy nbd+unix:///?socket=/tmp.nbd >/dev/null > -// > +// ./simple_copy nbd+unix:///?socket=/tmp.nbd >/dev/null > package main > > import ( > diff --git a/golang/libnbd_220_opt_list_test.go b/golang/libnbd_220_opt_list_test.go > index 264a50a7..ca888829 100644 > --- a/golang/libnbd_220_opt_list_test.go > +++ b/golang/libnbd_220_opt_list_test.go > @@ -19,10 +19,10 @@ > package libnbd > > import ( > + "fmt" > "os" > "os/exec" > "testing" > - "fmt" > ) > > var exports []string > @@ -94,7 +94,7 @@ func Test220OptList(t *testing.T) { > if count != 2 { > t.Fatalf("unexpected count after opt_list") > } > - if len(exports) != 2 || exports[0] != "a" || exports[1] != "b" { > + if len(exports) != 2 || exports[0] != "a" || exports[1] != "b" { > t.Fatalf("unexpected exports contents after opt_list") > } > }) > diff --git a/golang/libnbd_240_opt_list_meta_test.go b/golang/libnbd_240_opt_list_meta_test.go > index 011b5704..afbeb01b 100644 > --- a/golang/libnbd_240_opt_list_meta_test.go > +++ b/golang/libnbd_240_opt_list_meta_test.go > @@ -19,7 +19,7 @@ > package libnbd > > import ( > - "fmt"; > + "fmt" > "testing" > ) > > @@ -31,7 +31,7 @@ func listmetaf(user_data int, name string) int { > panic("expected user_data == 42") > } > list_count++ > - if (name == CONTEXT_BASE_ALLOCATION) { > + if name == CONTEXT_BASE_ALLOCATION { > list_seen = true > } > return 0 > @@ -62,7 +62,7 @@ func Test240OptListMeta(t *testing.T) { > list_count = 0 > list_seen = false > r, err := h.OptListMetaContext(func(name string) int { > - return listmetaf(42, name) > + return listmetaf(42, name) > }) > if err != nil { > t.Fatalf("could not request opt_list_meta_context: %s", err) > @@ -80,7 +80,7 @@ func Test240OptListMeta(t *testing.T) { > t.Fatalf("could not request add_meta_context: %s", err) > } > r, err = h.OptListMetaContext(func(name string) int { > - return listmetaf(42, name) > + return listmetaf(42, name) > }) > if err != nil { > t.Fatalf("could not request opt_list_meta_context: %s", err) > @@ -111,7 +111,7 @@ func Test240OptListMeta(t *testing.T) { > t.Fatalf("wrong result of get_meta_context: %s", *tmp) > } > r, err = h.OptListMetaContext(func(name string) int { > - return listmetaf(42, name) > + return listmetaf(42, name) > }) > if err != nil { > t.Fatalf("could not request opt_list_meta_context: %s", err) > @@ -121,7 +121,7 @@ func Test240OptListMeta(t *testing.T) { > } > > /* Fourth pass: opt_list_meta_context is stateless, so it should > - * not wipe status learned during opt_info > + * not wipe status learned during opt_info > */ > list_count = 0 > list_seen = false > @@ -137,13 +137,13 @@ func Test240OptListMeta(t *testing.T) { > if err != nil { > t.Fatalf("opt_info failed unexpectedly: %s", err) > } > - size, err := h.GetSize() > + size, err := h.GetSize() > if err != nil { > t.Fatalf("get_size failed unexpectedly: %s", err) > } > - if size != 1048576 { > + if size != 1048576 { > t.Fatalf("get_size gave wrong size") > - } > + } > meta, err := h.CanMetaContext(CONTEXT_BASE_ALLOCATION) > if err != nil { > t.Fatalf("can_meta_context failed unexpectedly: %s", err) > @@ -160,7 +160,7 @@ func Test240OptListMeta(t *testing.T) { > t.Fatalf("could not request add_meta_context: %s", err) > } > r, err = h.OptListMetaContext(func(name string) int { > - return listmetaf(42, name) > + return listmetaf(42, name) > }) > if err != nil { > t.Fatalf("could not request opt_list_meta_context: %s", err) > @@ -168,13 +168,13 @@ func Test240OptListMeta(t *testing.T) { > if r != 0 || r != list_count || list_seen { > t.Fatalf("unexpected count after opt_list_meta_context") > } > - size, err = h.GetSize() > + size, err = h.GetSize() > if err != nil { > t.Fatalf("get_size failed unexpectedly: %s", err) > } > - if size != 1048576 { > + if size != 1048576 { > t.Fatalf("get_size gave wrong size") > - } > + } > meta, err = h.CanMetaContext(CONTEXT_BASE_ALLOCATION) > if err != nil { > t.Fatalf("can_meta_context failed unexpectedly: %s", err) > @@ -192,7 +192,7 @@ func Test240OptListMeta(t *testing.T) { > t.Fatalf("could not request add_meta_context: %s", err) > } > r, err = h.OptListMetaContext(func(name string) int { > - return listmetaf(42, name) > + return listmetaf(42, name) > }) > if err != nil { > t.Fatalf("could not request opt_list_meta_context: %s", err) > @@ -241,7 +241,7 @@ func Test240OptListMeta(t *testing.T) { > list_count = 0 > list_seen = false > r, err = h.OptListMetaContext(func(name string) int { > - return listmetaf(42, name) > + return listmetaf(42, name) > }) > if err != nil { > bytes2, err2 := h.StatsBytesSent() > @@ -268,7 +268,7 @@ func Test240OptListMeta(t *testing.T) { > list_count = 0 > list_seen = false > r, err = h.OptListMetaContext(func(name string) int { > - return listmetaf(42, name) > + return listmetaf(42, name) > }) > if err != nil { > t.Fatalf("could not request opt_list_meta_context: %s", err) > diff --git a/golang/libnbd_245_opt_list_meta_queries_test.go b/golang/libnbd_245_opt_list_meta_queries_test.go > index 3ae2d854..1451b2fc 100644 > --- a/golang/libnbd_245_opt_list_meta_queries_test.go > +++ b/golang/libnbd_245_opt_list_meta_queries_test.go > @@ -28,7 +28,7 @@ func listmetaqf(user_data int, name string) int { > panic("expected user_data == 42") > } > listq_count++ > - if (name == CONTEXT_BASE_ALLOCATION) { > + if name == CONTEXT_BASE_ALLOCATION { > listq_seen = true > } > return 0 > @@ -64,9 +64,9 @@ func Test245OptListMetaQueries(t *testing.T) { > if err != nil { > t.Fatalf("could not request add_meta_context: %s", err) > } > - r, err := h.OptListMetaContextQueries([]string{ }, > + r, err := h.OptListMetaContextQueries([]string{}, > func(name string) int { > - return listmetaqf(42, name) > + return listmetaqf(42, name) > }) > if err != nil { > t.Fatalf("could not request opt_list_meta_context_queries: %s", err) > @@ -82,9 +82,9 @@ func(name string) int { > if err != nil { > t.Fatalf("could not request add_meta_context: %s", err) > } > - r, err = h.OptListMetaContextQueries([]string{ "x-nosuch:" }, > + r, err = h.OptListMetaContextQueries([]string{"x-nosuch:"}, > func(name string) int { > - return listmetaqf(42, name) > + return listmetaqf(42, name) > }) > if err != nil { > t.Fatalf("could not request opt_list_meta_context_queries: %s", err) > @@ -97,9 +97,9 @@ func(name string) int { > listq_count = 0 > listq_seen = false > r, err = h.OptListMetaContextQueries([]string{ > - "x-nosuch:", CONTEXT_BASE_ALLOCATION }, > + "x-nosuch:", CONTEXT_BASE_ALLOCATION}, > func(name string) int { > - return listmetaqf(42, name) > + return listmetaqf(42, name) > }) > if err != nil { > t.Fatalf("could not request opt_list_meta_context_queries: %s", err) > diff --git a/golang/libnbd_250_opt_set_meta_test.go b/golang/libnbd_250_opt_set_meta_test.go > index 4263ede5..d2068716 100644 > --- a/golang/libnbd_250_opt_set_meta_test.go > +++ b/golang/libnbd_250_opt_set_meta_test.go > @@ -28,7 +28,7 @@ func setmetaf(user_data int, name string) int { > panic("expected user_data == 42") > } > set_count++ > - if (name == CONTEXT_BASE_ALLOCATION) { > + if name == CONTEXT_BASE_ALLOCATION { > set_seen = true > } > return 0 > diff --git a/golang/libnbd_255_opt_set_meta_queries_test.go b/golang/libnbd_255_opt_set_meta_queries_test.go > index 232560bc..bdfacd63 100644 > --- a/golang/libnbd_255_opt_set_meta_queries_test.go > +++ b/golang/libnbd_255_opt_set_meta_queries_test.go > @@ -28,7 +28,7 @@ func setmetaqf(user_data int, name string) int { > panic("expected user_data == 42") > } > setq_count++ > - if (name == CONTEXT_BASE_ALLOCATION) { > + if name == CONTEXT_BASE_ALLOCATION { > setq_seen = true > } > return 0 > @@ -101,7 +101,7 @@ func(name string) int { > r, err = h.OptSetMetaContextQueries([]string{ > "x-nosuch:context", CONTEXT_BASE_ALLOCATION}, > func(name string) int { > - return setmetaqf(42, name) > + return setmetaqf(42, name) > }) > if err != nil { > t.Fatalf("could not request opt_set_meta_context_queries: %s", err) > diff --git a/golang/libnbd_405_pread_structured_test.go b/golang/libnbd_405_pread_structured_test.go > index ac4cb3e5..ac7d834a 100644 > --- a/golang/libnbd_405_pread_structured_test.go > +++ b/golang/libnbd_405_pread_structured_test.go > @@ -26,7 +26,7 @@ > var expected405 = make([]byte, 512) > > func psf(user_data int, buf2 []byte, offset uint64, status uint, > - error *int) int { > + error *int) int { > if user_data != 42 { > panic("expected user_data == 42") > } > @@ -68,10 +68,10 @@ func Test405PReadStructured(t *testing.T) { > > buf := make([]byte, 512) > err = h.PreadStructured(buf, 0, > - func(buf2 []byte, offset uint64, status uint, > - error *int) int { > - return psf(42, buf2, offset, status, error) > - }, nil) > + func(buf2 []byte, offset uint64, status uint, > + error *int) int { > + return psf(42, buf2, offset, status, error) > + }, nil) > if err != nil { > t.Fatalf("%s", err) > } > diff --git a/golang/libnbd_590_aio_copy_test.go b/golang/libnbd_590_aio_copy_test.go > index c22653a7..e8c32dc0 100644 > --- a/golang/libnbd_590_aio_copy_test.go > +++ b/golang/libnbd_590_aio_copy_test.go > @@ -28,8 +28,11 @@ > var bytes_read = uint(0) > var bytes_written = uint(0) > > -/* Functions to handle FdSet. > - XXX These probably only work on 64 bit platforms. */ > +/* > +Functions to handle FdSet. > + > + XXX These probably only work on 64 bit platforms. > +*/ > func fdset_set(set *syscall.FdSet, fd int) { > (*set).Bits[fd/64] |= 1 << (uintptr(fd) % 64) > } > @@ -56,8 +59,11 @@ type wbuf struct { > > var writes []wbuf > > -/* Called whenever any asynchronous pread command from > - the source has completed. */ > +/* > +Called whenever any asynchronous pread command from > + > + the source has completed. > +*/ > func read_completed(buf AioBuffer, offset uint64) int { > bytes_read += buf.Size > /* Move the AIO buffer to the write queue. */ > @@ -66,8 +72,11 @@ func read_completed(buf AioBuffer, offset uint64) int { > return 1 > } > > -/* Called whenever any asynchronous pwrite command to the > - destination has completed. */ > +/* > +Called whenever any asynchronous pwrite command to the > + > + destination has completed. > +*/ > func write_completed(buf AioBuffer) int { > bytes_written += buf.Size > /* Now we have to manually free the AIO buffer. */ > diff --git a/golang/libnbd_620_stats_test.go b/golang/libnbd_620_stats_test.go > index 7b4ba009..63667160 100644 > --- a/golang/libnbd_620_stats_test.go > +++ b/golang/libnbd_620_stats_test.go > @@ -124,16 +124,16 @@ func Test620Stats(t *testing.T) { > t.Fatalf("%s", err) > } > > - if bs2 != bs1 + 28 { > + if bs2 != bs1+28 { > t.Fatalf("unexpected value for bs2") > } > - if cs2 != cs1 + 1 { > + if cs2 != cs1+1 { > t.Fatalf("unexpected value for cs2") > } > - if br2 != br1 + 16 { /* assumes nbdkit uses simple reply */ > + if br2 != br1+16 { /* assumes nbdkit uses simple reply */ > t.Fatalf("unexpected value for br2") > } > - if cr2 != cr1 + 1 { > + if cr2 != cr1+1 { > t.Fatalf("unexpected value for cr2") > } > > @@ -169,13 +169,13 @@ func Test620Stats(t *testing.T) { > if bs3 <= bs2 { > t.Fatalf("unexpected value for bs3") > } > - if cs3 != cs2 + 1 { > + if cs3 != cs2+1 { > t.Fatalf("unexpected value for cs3") > } > if br3 < br2 { > t.Fatalf("unexpected value for br3") > } > - if cr3 != cr2 + slop { > + if cr3 != cr2+slop { > t.Fatalf("unexpected value for cr3") > } > }I've done nearly nothing in Go, and generally the updates look justified... but the changes to "libnbd_620_stats_test.go" are hideous. Who on Earth considers if cr3 != cr2+slop superior to if cr3 != cr2 + slop ??? *shudder* Question in passing: if we wrote if cr3 != (cr2 + slop) would gofmt contract that too to if cr3 != (cr2+slop) ? Asking because I suspect that gofmt's purpose with the whitespace removal around "+" is to emphasize that "+" binds more strongly than "!=". And, if we forced the binding with () around +, then gofmt might not feel the need to emphasize the difference between the binding strengths. Acked-by: Laszlo Ersek <lersek at redhat.com> Laszlo
Eric Blake
2023-Jul-27 14:50 UTC
[Libguestfs] [libnbd PATCH 5/7] golang: Use 'gofmt' style recommendations on manual files
On Thu, Jul 27, 2023 at 01:52:00PM +0200, Laszlo Ersek wrote:> On 7/26/23 19:50, Eric Blake wrote: > > I ran: > > gofmt -s -w $(git ls-files -- '**/*.go') > > > > then touched up a few comments in test 590 where it mis-interpreted > > our intentions of having a single sentence that occupies more than 80 > > columns. > > > > Most of the changes are whitespace fixes (consistent use of TAB, > > altering the whitespace around operators), using preferred ordering > > for imports, and eliding excess syntax (unneeded () or ;). > > > > This patch only adjusts files directly in git control. Later patches > > will tackle (most) Go style problems in the generated files, then > > automate a way to ensure we don't regress. > > > > Signed-off-by: Eric Blake <eblake at redhat.com> > > --- > > +++ b/golang/libnbd_620_stats_test.go > > @@ -124,16 +124,16 @@ func Test620Stats(t *testing.T) { > > t.Fatalf("%s", err) > > } > > > > - if bs2 != bs1 + 28 { > > + if bs2 != bs1+28 { > > t.Fatalf("unexpected value for bs2") > > }> > - if cr3 != cr2 + slop { > > + if cr3 != cr2+slop { > > t.Fatalf("unexpected value for cr3") > > } > > } > > I've done nearly nothing in Go, and generally the updates look > justified... but the changes to "libnbd_620_stats_test.go" are hideous. > Who on Earth considers > > if cr3 != cr2+slop > > superior to > > if cr3 != cr2 + slop > > ??? > > *shudder*That's gofmt for you. I was surprised by it too.> > Question in passing: if we wrote > > if cr3 != (cr2 + slop) > > would gofmt contract that too to > > if cr3 != (cr2+slop)Equally surprising, gofmt (at least from golang-bin-1.20.6-1.fc38) does NOT remove the spaces from that format, nor does it complain that the () are spurious. Of course, future gofmt may change style again, but now we have a regression test in place to catch that. And we may be faced with decisions down the road on how to pacify cases where gofmt itself changes styles from one release of the language to the next (if that happens, that would be a strong argument that we should remove gofmt from a gating 'make check' test, and instead incorporate it as an optional 'make dist' step - if we reach a point where there is no longer "the" canonical formatting, we shouls still favor tarballs created with "a" canoncial format). For now, I'm happy to squash in: diff --git i/golang/libnbd_620_stats_test.go w/golang/libnbd_620_stats_test.go index 63667160..6cc3cdc1 100644 --- i/golang/libnbd_620_stats_test.go +++ w/golang/libnbd_620_stats_test.go @@ -124,16 +124,16 @@ func Test620Stats(t *testing.T) { t.Fatalf("%s", err) } - if bs2 != bs1+28 { + if bs2 != (bs1 + 28) { t.Fatalf("unexpected value for bs2") } - if cs2 != cs1+1 { + if cs2 != (cs1 + 1) { t.Fatalf("unexpected value for cs2") } - if br2 != br1+16 { /* assumes nbdkit uses simple reply */ + if br2 != (br1 + 16) { /* assumes nbdkit uses simple reply */ t.Fatalf("unexpected value for br2") } - if cr2 != cr1+1 { + if cr2 != (cr1 + 1) { t.Fatalf("unexpected value for cr2") } @@ -169,13 +169,13 @@ func Test620Stats(t *testing.T) { if bs3 <= bs2 { t.Fatalf("unexpected value for bs3") } - if cs3 != cs2+1 { + if cs3 != (cs2 + 1) { t.Fatalf("unexpected value for cs3") } if br3 < br2 { t.Fatalf("unexpected value for br3") } - if cr3 != cr2+slop { + if cr3 != (cr2 + slop) { t.Fatalf("unexpected value for cr3") } } as well as amending the commit message to mention the manual touchup needed to work around the (in our opinion, misguided) advice from Go.> > ? > > Asking because I suspect that gofmt's purpose with the whitespace > removal around "+" is to emphasize that "+" binds more strongly than > "!=". And, if we forced the binding with () around +, then gofmt might > not feel the need to emphasize the difference between the binding strengths. > > Acked-by: Laszlo Ersek <lersek at redhat.com> > > Laszlo >-- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org