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
New lint:
manual_midpoint
#13851base: master
Are you sure you want to change the base?
New lint:
manual_midpoint
#13851Changes from all commits
e9c2d82
8fc52b4
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.
Interesting choice of form, I would have probably use the method syntax (ie.
.midpoint(..)
) instead, as to not have to much shifts from the original code, but that works as well.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 feel that linting
(a + b) / 2
asa.midpoint(b)
introduces an asymmetry betweena
andb
whileu32::midpoint(a, b)
doesn't. But I'm not opposed to this change.What do others think? Please upvote if you prefer
a.midpoint(b)
and downvote if you preferu32::midpoint(a, b)
.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 can't pick between the 2 because the first one is more convenirnt but the second one is more readable...
Well, by writing this comment I realized I prefer more readable code so let's go for 2.
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.
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'm reluctant to add this on the lint message, because this particular use may well never overflow depending on the context. This is appropriate for the lint short description though, I'll add it there. What do you think?
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 should add it to the lint short description, but I still think we should mention it in the main message as it's the main issue (not the fact that it's a manual implementation). We could reduce the assertion by saying "which may overflow" (instead of "can").
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.
Any addition may overflow, and we do not suggest using
.checked_add()
. I'll let other weigh in, but I'm not comfortable saying "which may overflow" on(a + b) / 2
ifa
andb
are indices in a vector for example, they will never overflow as they will be nowhere nearusize::MAX/2
for any realistic vector.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.
u8::MAX = 255
,u16::MAX = 65536
are fairly realistic numbers to me (which could be indices of custom containers).reminder of https://research.google/blog/extra-extra-read-all-about-it-nearly-all-binary-searches-and-mergesorts-are-broken/
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 was specifically talking about the
usize
case which is used to index a vector. And let's not confuse the issue: I'm not trying to argue that we should not lint, we will, only that I am not comfortable saying that a particular computation may overflow.I couldn't find lints that warn about the potential risk when linting an expression if the risk is remote or non-existing for this particular expression, even though they still lint to make it clearer and safer should this code be changed later.