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

Tiny code simplification in codec #54

Merged
1 commit merged into from Sep 14, 2018
Merged

Tiny code simplification in codec #54

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion codec.go
View file
Open in desktop
Expand Up @@ -125,7 +125,7 @@ func (u *UUID) decodeCanonical(t []byte) error {
return fmt.Errorf("uuid: incorrect UUID format %s", t)
}

src := t[:]
Copy link
Member

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?

Copy link
Member

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

Copy link
Member

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.

Copy link
Member

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

Copy link
Member

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.

Copy link
Member

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.

src := t
dst := u[:]

for i, byteGroup := range byteGroups {
Expand Down