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/go: refactor mvsReqs.Max for better readability #39042

Closed
robpike opened this issue May 13, 2020 · 4 comments
Closed

cmd/go: refactor mvsReqs.Max for better readability #39042

robpike opened this issue May 13, 2020 · 4 comments

Comments

@robpike
Copy link
Contributor

@robpike robpike commented May 13, 2020

At head (go version devel +e90b0ce68b Sat May 2 20:22:19 2020 +0000 darwin/amd64) I see this function in src/go/cmd/go/internal/modload/mvs.go:

func (*mvsReqs) Max(v1, v2 string) string {
	if v1 != "" && semver.Compare(v1, v2) == -1 {
		return v2
	}
	return v1
}

I believe that "return v1" is wrong if v1 is empty, and the code should be something like:

func (*mvsReqs) Max(v1, v2 string) string {
	if v1 == "" || semver.Compare(v1, v2) == -1 {
		return v2
	}
	return v1
}

but I am not confident of the semantics this function requires. Moreover, since Compare protects against bad parses, it's possible no checking is required:

func (*mvsReqs) Max(v1, v2 string) string {
	if semver.Compare(v1, v2) == -1 {
		return v2
	}
	return v1
}
@myitcv
Copy link
Member

@myitcv myitcv commented May 13, 2020

@jayconrod
Copy link
Contributor

@jayconrod jayconrod commented May 13, 2020

I think this is working as intended, but the code could be refactored for better readability.

mvsReqs.Max is run as part of MVS. At the point it's called, all go.mod files have been read, and all versions have been canonicalized. The only module with the version "" should be the main module, which must be chosen over any other version required by its transitive dependencies. For this reason, "" compares higher than any other string.

We should refactor this for better readability. We should explain why semver.Max is not used instead.

@jayconrod jayconrod changed the title cmd/go: bug in mvsReqs.Max? cmd/go: refactor mvsReqs.Max for better readability May 13, 2020
@jayconrod jayconrod added this to the Backlog milestone May 13, 2020
@bcmills
Copy link
Member

@bcmills bcmills commented May 13, 2020

We should explain why semver.Max is not used instead.

#32700 is another reason we avoid semver.Max.

@gopherbot
Copy link

@gopherbot gopherbot commented May 14, 2020

Change https://golang.org/cl/234003 mentions this issue: cmd/go/internal/modload: document mvsReqs.Max

@gopherbot gopherbot closed this in 8be0de1 Jun 1, 2020
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
5 participants
You can’t perform that action at this time.