buf_encoder: change "early range check" assertion on max to one on min - #262
Conversation
|
On d1874c8 successfully ran local tests |
|
cc @Kixunil when you get a chance can you look at this? Alternately, it might be easier for you to just fix the PL issues and I can review them. (I'm happy either way, but I think reviewing is usually harder, or at least more tiring, than writing code.) |
Kixunil
left a comment
Kixunil
left a comment
There was a problem hiding this comment.
This is correct, just the explanation is a bit misleading. Iterators are allowed to understate their length, in which case this will only lose performance optimization and has no other problems because of transitivity of <= operator. Overstating min is a bug in the iterator in which case yes, we're allowed to panic but they are allowed to do it in the UB sense - it will not cause UB and we must not rely on it. So here, technically the only thing that potentially causes the optimization is monomorphization + compiler inlining enough to see that min is actually the same thing as len and then see that any checks in put_byte are also related to len and that these checks are dead.
|
I don't understand the difference between what you said and what I said. Are you asking me to change the PR description and/or commit message? Can you suggest alternate text? |
|
I guess you don't like me saying iterators are "allowed" to overstate their minimum when this is clearly a violation of the |
|
Yes, that's really the part that I'd prefer to change. Maybe "(Iterators are allowed to understate the minimum which does NOT cause panics here and overstating is a bug in the iterator but not unsound, so we are allowed to panic on it but we MUST NOT cause UB.)" |
|
What do you think of the updated PR description. |
|
It's good but could you please change the commit message too? |
…`min` We have an assertion in `buf_encoder` whose purpose is to provide an early range-check to (hopefully) prod the compiler into eliding range checks inside a loop. But because we were asserting on the iterator's `size_hint` maximum, which might overstate the iterator's length, this extra assertion accidentally restricted what iterators could be used with the function. For the cases we can realistically expect optimizations, the minimum and maximum are the same, so just use the minimum instead. (If iterators understate their minimums, there is no problem except that our optimization fails; if they overstate their minimums, they are misbehaving and we are within our right to panic, which we do.) (Though we would not be within our rights to do something unsound. Which we don't.)
d1874c8 to
43b30bf
Compare
|
Done. |
|
On 43b30bf successfully ran local tests |
2 participants
We have an assertion in
buf_encoderwhose purpose is to provide an early range-check to (hopefully) prod the compiler into eliding range checks inside a loop.But because we were asserting on the iterator's
size_hintmaximum, which might overstate the iterator's length, this extra assertion accidentally restricted what iterators could be used with the function.For the cases we can realistically expect optimizations, the minimum and maximum are the same, so just use the minimum instead. (If iterators understate their minimums, there is no problem except that our optimization fails; if they overstate their minimums, they are misbehaving and we are within our right to panic, which we do.) (Though we would not be within our rights to do something unsound. Which we don't.)
Fixes https://github.com/project-loupe/audit-hex-conservative/issues/3