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/vet: update for new number formats #29986

Closed
rsc opened this issue Jan 30, 2019 · 22 comments
Closed

cmd/vet: update for new number formats #29986

rsc opened this issue Jan 30, 2019 · 22 comments

Comments

@rsc
Copy link
Contributor

@rsc rsc commented Jan 30, 2019

cmd/vet once again lives outside the main tree (sigh).

We need to remember to update the printf checker for
the new formats %#b and %O on integers, %x on float/complex.

@rsc rsc added this to the Go1.13 milestone Jan 30, 2019
@gopherbot
Copy link

@gopherbot gopherbot commented Jan 30, 2019

Change https://golang.org/cl/160246 mentions this issue: fmt: format 0b, 0o prefixes in %#b and %O

@gopherbot
Copy link

@gopherbot gopherbot commented Jan 30, 2019

Change https://golang.org/cl/160245 mentions this issue: fmt: format hex floats and complexes

@andybons andybons added the NeedsFix label Feb 6, 2019
gopherbot pushed a commit that referenced this issue Feb 26, 2019
This CL modifies fmt's printer to implement %x and %X
for formatting floating-point data (floats and complexes)
in standard hexadecimal notation.

See golang.org/design/19308-number-literals for background.

For #29008.
Vet update is #29986.

Change-Id: If2842a11631bc393a1ebcf6914ed07658652af5a
Reviewed-on: https://go-review.googlesource.com/c/160245
Reviewed-by: Robert Griesemer <gri@golang.org>
Reviewed-by: Rob Pike <r@golang.org>
gopherbot pushed a commit that referenced this issue Feb 26, 2019
This CL modifies fmt's printer to implement %#b and %O
to emit leading 0b and 0o prefixes on binary and octal.
(%#o is already taken and emits "0377"; %O emits "0o377".)

See golang.org/design/19308-number-literals for background.

For #19308.
For #12711.
Vet update is #29986.

Change-Id: I7c38a4484c48a03abe9f6d45c7d981c7c314f583
Reviewed-on: https://go-review.googlesource.com/c/160246
Reviewed-by: Robert Griesemer <gri@golang.org>
Reviewed-by: Rob Pike <r@golang.org>
@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@seh
Copy link
Contributor

@seh seh commented Dec 21, 2019

It looks like vet's update for the new octal format verb 'O' would go here.

@gopherbot
Copy link

@gopherbot gopherbot commented May 26, 2020

Change https://golang.org/cl/235100 mentions this issue: go/analysis/passes/printf: allow %O in format strings

gopherbot pushed a commit to golang/tools that referenced this issue May 26, 2020
%O is supported since Go 1.13. See golang.org/design/19308-number-literals for
the background.

Support for %O has been added by copying and adapting the %o implementation.

Updates golang/go#29986

Change-Id: Ic49d3cc8d9aefcc0ecbfcfe5ebf206e6f951d413
Reviewed-on: https://go-review.googlesource.com/c/tools/+/235100
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@michael-schaller
Copy link
Contributor

@michael-schaller michael-schaller commented May 27, 2020

As far as I can tell this bug should be fixed.

@rsc, could you double check and mark this bug as fixed?
https://github.com/golang/tools/blob/8b020aee10d2dc97e13b76ce01b59c9b5d4a4d0f/go/analysis/passes/printf/printf.go#L790-L819

@michael-schaller
Copy link
Contributor

@michael-schaller michael-schaller commented May 27, 2020

Also as %O was introduced with Go 1.13 should this be included in the next Go 1.13 and 1.14 point releases?

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented May 27, 2020

I don't think this is fixed until we import a new version of the packages into the main sources.

@ianlancetaylor

This comment has been minimized.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented May 27, 2020

@gopherbot Please open backport issues for 1.13 and 1.14

"go vet" should permit %O in fmt.Printf formats, as the formatting code was added in the 1.13 release.

@gopherbot
Copy link

@gopherbot gopherbot commented May 27, 2020

Backport issue(s) opened: #39287 (for 1.13), #39288 (for 1.14).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

@michael-schaller
Copy link
Contributor

@michael-schaller michael-schaller commented May 28, 2020

@ianlancetaylor note that %#b and %x support was added in other changes by other people and these changes would need to be cherry picked as well...

The changes in question are:

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented May 28, 2020

@michael-schaller True. At a quick glance it seems like those two are fixed in 1.14, but not in 1.13.

@michael-schaller
Copy link
Contributor

@michael-schaller michael-schaller commented May 29, 2020

@ianlancetaylor please let me know if I can help in some other way with this bug. I'm curious how this is all handled... ;-)

@dmitshur
Copy link
Member

@dmitshur dmitshur commented May 29, 2020

The version of the golang.org/x/tools module is specified in the src/cmd/go.mod files on the release branches for Go 1.14 and 1.13:

The release branches in golang.org/x/tools should correspond to those pseudo-versions:

This is already the case for 1.14, but the x/tools release-branch.go1.13 is incorrect (it's many commits ahead) and will need to be reset to match the version selected in https://github.com/golang/go/blob/release-branch.go1.13/src/cmd/go.mod#L11.

Then, it's a matter of backporting the relevant vet fixes to each of the x/tools release branches, and updating the go.mod (and re-running go mod vendor as documented here) accordingly on the main Go release branches.

Given it's quite a bit of mechanics, it's likely a good idea to wait until the backport issues (#39288 and #39287) make progress from their current CherryPickCandidate state before starting the work.

@dmitshur dmitshur modified the milestones: Backlog, Go1.15 Jun 2, 2020
@dmitshur
Copy link
Member

@dmitshur dmitshur commented Jun 2, 2020

The backport issues have been approved.

I'll also add release-blocker to this to track the vendor of CL 235100 into the main tree.

@dmitshur dmitshur self-assigned this Jun 2, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Jun 2, 2020

Change https://golang.org/cl/236138 mentions this issue: cmd: update golang.org/x/tools to v0.0.0-20200601175630-2caf76543d99

@dmitshur dmitshur removed their assignment Jun 2, 2020
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 2, 2020

@dmitshur Sorry, didn't notice that you had assigned this to yourself.

@dmitshur
Copy link
Member

@dmitshur dmitshur commented Jun 2, 2020

@ianlancetaylor That is not a problem. I noticed you've sent the CL thanks to gopherbot's message above just before I started doing any work. I reviewed your CL. Thank you.

@gopherbot
Copy link

@gopherbot gopherbot commented Jun 2, 2020

Change https://golang.org/cl/236161 mentions this issue: [release-branch.go1.14] go/analysis/passes/printf: allow %O in format strings

@gopherbot gopherbot closed this in 1193958 Jun 2, 2020
gopherbot pushed a commit to golang/tools that referenced this issue Jun 2, 2020
… strings

%O is supported since Go 1.13. See golang.org/design/19308-number-literals for
the background.

Support for %O has been added by copying and adapting the %o implementation.

For golang/go#39288.
For golang/go#29986.

Change-Id: Ic49d3cc8d9aefcc0ecbfcfe5ebf206e6f951d413
Reviewed-on: https://go-review.googlesource.com/c/tools/+/235100
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
(cherry picked from commit 8b020ae)
Reviewed-on: https://go-review.googlesource.com/c/tools/+/236161
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Jun 3, 2020

Change https://golang.org/cl/236199 mentions this issue: [release-branch.go1.14] cmd: update golang.org/x/tools to v0.0.0-20200602230032-c00d67ef29d0

@gopherbot
Copy link

@gopherbot gopherbot commented Jun 15, 2020

Change https://golang.org/cl/237945 mentions this issue: [release-branch.go1.13] go/analysis/passes/printf: allow %O in format strings

gopherbot pushed a commit to golang/tools that referenced this issue Jun 15, 2020
… strings

%O is supported since Go 1.13. See golang.org/design/19308-number-literals for
the background.

Support for %O has been added by copying and adapting the %o implementation.

Updates golang/go#29986.
For golang/go#39287.

Change-Id: Ic49d3cc8d9aefcc0ecbfcfe5ebf206e6f951d413
Reviewed-on: https://go-review.googlesource.com/c/tools/+/235100
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-on: https://go-review.googlesource.com/c/tools/+/237945
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Jun 15, 2020

Change https://golang.org/cl/238018 mentions this issue: [release-branch.go1.13] cmd: update golang.org/x/tools to v0.0.0-20200615191743-991d59a616de

gopherbot pushed a commit that referenced this issue Jun 15, 2020
…0602230032-c00d67ef29d0

This teaches vet to recognize %O in a fmt.Printf format string.
O has been supported since the 1.13 release, but vet would warn about it.

Fixes #39288.
For #29986.

Change-Id: Ia7817ee60ae6beac32cc402c0c68afa917e4ef0f
Reviewed-on: https://go-review.googlesource.com/c/go/+/236199
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
gopherbot pushed a commit that referenced this issue Jun 15, 2020
…0615191743-991d59a616de

This teaches vet to recognize the new formats %#b and %O on integers,
%x on float/complex. They have been supported since the 1.13 release,
but vet would warn about it.

Also update cmd/vet testdata accordingly, as was done in CL 202083.

Fixes #39287.
For #29986.

Change-Id: Ia7817ee60ae6beac32cc402c0c68afa917e4ef0f
Reviewed-on: https://go-review.googlesource.com/c/go/+/238018
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
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
7 participants
You can’t perform that action at this time.