-
-
Notifications
You must be signed in to change notification settings - Fork 1k
Bump minimum Go to 1.22 and use new features #1017
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
base: master
Are you sure you want to change the base?
Conversation
Go 1.21 brings us the builtin min func and slices.Contains. Go 1.22 brings us range over int and fixed for loop scoping. We were also able to drop the build tags from the path value code and inline the three-line function directly. This should still work on tinygo as they claim to support Go 1.24.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thank you!
I'll wait for more 👀 to review and then we can merge. 👍
// longestPrefix finds the length of the shared prefix | ||
// of two strings | ||
func longestPrefix(k1, k2 string) int { | ||
max := len(k1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interesting .. so this variable was named incorrectly - it meant "min" instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think the original naming was meant to imply max iterations as opposed to min string length. But I agree it was a little confusing.
// longestPrefix finds the length of the shared prefix of two strings | ||
func longestPrefix(k1, k2 string) (i int) { | ||
for i = 0; i < min(len(k1), len(k2)); i++ { | ||
if k1[i] != k2[i] { | ||
break | ||
} | ||
} | ||
return i | ||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing I noticed -- (unrelated to this PR) -- this function might not work well with unicode characters, as it works on bytes and not on runes.
The question is -- are we ever passing unicode strings into this function (e.g. "/café"
, "/café/1"
). Can this happen?
Go 1.21 brings us the builtin min func and slices.Contains.
Go 1.22 brings us range over int and fixed for loop scoping.
We were also able to drop the build tags from the path value code and inline the three-line function directly. This should still work on tinygo as they claim to support Go 1.24.