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.

Sign up for GitHub

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

Jump to bottom

x/tools/cmd/stringer: redundant check #36461

Closed
awilliams opened this issue Jan 8, 2020 · 4 comments
Closed

x/tools/cmd/stringer: redundant check #36461

awilliams opened this issue Jan 8, 2020 · 4 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
Unreleased

Comments

@awilliams
Copy link

What version of Go are you using (go version)?

$ go version
go version go1.13.5 darwin/amd64

Does this issue reproduce with the latest release?

This issue reproduces using x/tools/cmd/stringer on the latest from master (b9c20ae).

What operating system and processor architecture are you using (go env)?

go env Output
$ go env GOOS GOARCH
darwin
amd64

What did you do?

Ran x/tools/cmd/stringer on the following file: https://play.golang.org/p/H5tvJ6Edhrr

$ stringer -type=signedSingleRun,signedMultipleRuns,unsignedSingleRun,unsignedMultipleRuns -linecomment main.go

What did you expect to see?

stringer's output can be grouped into four categories based on the type being "stringed":

  • single run: the values are consecutive, e.g. 1,2,3,4
  • multiple runs: the values are nonconsecutive, e.g. 1,2,10,20
  • signed: type is signed
  • unsigned: type is unsigned

Using the above playground example:

  • For signed single run, the String method includes a check for i < 0:
func (i signedSingleRun) String() string {
	if i < 0 || i >= signedSingleRun(len(_signedSingleRun_index)-1) {
		return "signedSingleRun(" + strconv.FormatInt(int64(i), 10) + ")"
	}
	return _signedSingleRun_name[_signedSingleRun_index[i]:_signedSingleRun_index[i+1]]
}
  • For unsigned single run, the String method does not include this check:
func (i unsignedSingleRun) String() string {
	if i >= unsignedSingleRun(len(_unsignedSingleRun_index)-1) {
		return "unsignedSingleRun(" + strconv.FormatInt(int64(i), 10) + ")"
	}
	return _unsignedSingleRun_name[_unsignedSingleRun_index[i]:_unsignedSingleRun_index[i+1]]
}

I would expect to see the same logic for the signed/unsigned multiple runs cases.

What did you see instead?

Instead, the unsigned / multiple run case includes a redundant check for 0 <= i in its String method.

Using the above playground example:

  • For signed multiple runs:
func (i signedMultipleRuns) String() string {
	switch {
	case 0 <= i && i <= 2:
		return _signedMultipleRuns_name_0[_signedMultipleRuns_index_0[i]:_signedMultipleRuns_index_0[i+1]]
	case 103 <= i && i <= 105:
		i -= 103
		return _signedMultipleRuns_name_1[_signedMultipleRuns_index_1[i]:_signedMultipleRuns_index_1[i+1]]
	default:
		return "signedMultipleRuns(" + strconv.FormatInt(int64(i), 10) + ")"
	}
}
  • For unsigned multiple runs (comment added for emphasis):
func (i unsignedMultipleRuns) String() string {
	switch {
	case 0 <= i && i <= 2: // redundant 0 <= i check
		return _unsignedMultipleRuns_name_0[_unsignedMultipleRuns_index_0[i]:_unsignedMultipleRuns_index_0[i+1]]
	case 103 <= i && i <= 105:
		i -= 103
		return _unsignedMultipleRuns_name_1[_unsignedMultipleRuns_index_1[i]:_unsignedMultipleRuns_index_1[i+1]]
	default:
		return "unsignedMultipleRuns(" + strconv.FormatInt(int64(i), 10) + ")"
	}
}
@gopherbot gopherbot added this to the Unreleased milestone Jan 8, 2020
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Jan 8, 2020
@toothrot toothrot added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jan 8, 2020
@toothrot
Copy link
Contributor

toothrot commented Jan 8, 2020

/cc @mvdan

@mvdan
Copy link
Member

mvdan commented Jan 9, 2020

Sure. Want to send a CL with a test?

@awilliams
Copy link
Author

@mvdan I'd be happy to send a CL.

@gopherbot
Copy link

Change https://golang.org/cl/214180 mentions this issue: cmd/stringer: remove redundant check in generated code

jocgir pushed a commit to jocgir/gotools that referenced this issue Apr 29, 2020
@awilliams @mvdan
cmd/stringer: remove redundant check in generated code
ab256bf 
In a certain case, stringer would generate the following check for an
an unsigned integer i: "0 <= i". This changes stringer to not generate
such a check.

Also adds an additional test case for an unsigned multiple run that does
not include zero. There was already a case ("unum") that included an
unsigned multiple run starting at zero. This case's output was updated
accordingly.

Fixes golang/go#36461

Change-Id: I57e79384a0b802fa4571e2b3495db168b814bcaa
Reviewed-on: https://go-review.googlesource.com/c/tools/+/214180
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
@golang golang locked and limited conversation to collaborators Jan 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
4 participants
@toothrot @awilliams @mvdan @gopherbot