10:04 a.m.
On 7/27/23 16:50, Eric Blake wrote:
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(a)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.
Thank you :)
Laszlo
>
> ?
>
> 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(a)redhat.com>
>
> Laszlo
>