-
Notifications
You must be signed in to change notification settings - Fork 19
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
#411 use safer method when converting between byte and float slices #454
Conversation
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.
I think we just need a guard if statement, but everything else looks good 😁
hdr.Len = 8 * len(floats) | ||
hdr.Cap = 8 * cap(floats) | ||
return byts | ||
return unsafe.Slice((*byte)(unsafe.Pointer(&floats[0])), 8*len(floats)) |
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.
We need to be careful about the case where len(floats)
is 0
, otherwise floats[0]
will panic. We could just have a guard if statement checking that condition first (and simply return nil
in that case). That applies to bytesAsBytes
too.
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 🍰
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.
Thanks for the contribution!
Ahh, I can see what's happening with CI now. Because of the Go image upgrade for |
The `golang:1.17` image uses Debian bullseye as its base, which doesn't have the required version of GEOS available. By downgrading back to buster, GEOS is now available.
Description
We want to move away from editing slice headers when converting between byte and float slices. And to use the "safer" unsafe.slice method available in golang 1.17.
Benchmark comparisons current approach vs the new one
Using the new method while slower than the existing edit header approach is still considerably faster than using the standard library.
Check List
Have you:
Added unit tests?
Add cmprefimpl tests? (if appropriate?)
Updated release notes? (if appropriate?)
Related Issue
#411
Benchmark Results
run_benchmarks.sh
script.Click to expand