Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add GovernorCountingFractional #5045
Add GovernorCountingFractional #5045
Changes from 2 commits
729ef4c
b56632a
66f2b3b
459ce71
7cff638
12123b0
bc9f970
93f460f
e792599
5ae1d9e
2bf4d2d
f9db044
7ffc6d2
e4d4fad
19c8142
0420d9a
8e95e96
95f3fb2
68033f5
37e9a0e
dce6fba
e0f5b71
f1b71bc
37391ce
72a420b
149fe65
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 function concerns me because the length of
data
is ignored andpos
may be reading out of bounds. The assembly block correctly omits thememory-safe
annotation, but like I said in a recent issue this globally disables optimizations so it should be avoided.Is it possible to just check the validity of
pos
against the data length? Sadly I do see it would make the code less efficient by checking the length three times... Perhaps the helper should directly unpack all three parameters.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 having the helper to unpack all three parameters is the easiest to ensure memory safetyness.
I'll update with this alternative
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 is private, and everywhere we call it we first validate the length. Is that not good enough?
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.
GIven how frequent it is for people to copy-paste from our code I think we should try to make functions self-contained and well documented even if they're private.
In this case, not only that I think this is the cleanest way of making the assembly block
memory-safe
to avoid disabling optimizations, but also I share the concern with @frangio; although the function is unreachable with invalid parameters, I don't feel comfortable having such a function around.