-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Fix copy arguments for strict aligned architectures #36976
Conversation
This pull request does not have a backport label.
To fixup this pull request, you need to add the backport labels for the needed
|
This pull request is now in conflicts. Could you fix it? 🙏
|
Small typo. In practice only affects arm32 and arm64 as I doubt there are users of other architectures around.
@@ -16,7 +16,7 @@ import ( | |||
var errBadSize = errors.New("bad size for integer") | |||
|
|||
func copyInt(dst unsafe.Pointer, src unsafe.Pointer, len uint8) error { | |||
copy((*(*[maxIntSizeBytes]byte)(src))[:len], (*(*[maxIntSizeBytes]byte)(src))[:len]) | |||
copy((*(*[maxIntSizeBytes]byte)(dst))[:len], (*(*[maxIntSizeBytes]byte)(src))[:len]) |
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 can be brought into the modern era and have error handling added if it is
func copyInt(dst unsafe.Pointer, src unsafe.Pointer, len uint8) error {
switch len {
case 1, 2, 4, 8:
copy(unsafe.Slice((*byte)(dst), len), unsafe.Slice((*byte)(src), len))
return nil
default:
return errBadSize
}
}
(we can also remove the array punning below by changing the preamble in readInt
to asSlice := unsafe.Slice((*byte)(ptr), len)
.
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.
That's great, but can we do this in a second future step? I've found this just by reading the code and don't have a proper setup in place to test more invasive changes.
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, fair enough.
elastic#36976) Small typo. In practice only affects arm32 and arm64 as I doubt there are users of other architectures around.
Proposed commit message
Checklist
My code follows the style guidelines of this projectI have commented my code, particularly in hard-to-understand areasI have made corresponding changes to the documentationI have made corresponding change to the default configuration filesI have added tests that prove my fix is effective or that my feature worksCHANGELOG.next.asciidoc
orCHANGELOG-developer.next.asciidoc
.