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
Tiny code simplification in codec #54
Conversation
2 similar comments
@@ -125,7 +125,7 @@ func (u *UUID) decodeCanonical(t []byte) error { | |||
return fmt.Errorf("uuid: incorrect UUID format %s", t) | |||
} | |||
|
|||
src := t[:] |
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.
Could this be an optimization? I recall a talk from today talking about "this being on the stack" now IIRC.
Can we benchmark before and after?
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.
Both are values on stack I presume
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.
The t value is already a slice i think. The [:]
should be a no-op.
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.
It looks like a no-op indeed https://go.godbolt.org/z/OuMHTy
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.
What's the reason for even doing it? Might it make more sense just to put src
in the function declaration and omit the line altogether? It's not a breaking API change.
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.
@theckman Unfortunately, that would mean we lose consistency with the rest of the decodeX
methods in that file, which all take a t []byte
parameter.
FWIW, I have no opinion on this. I think the proposed change is fine as-is.
Drop unnecessary slice since type is already a slice.