Fix countdown through 0 for uint32
#19926
Conversation
- `countdown` on an `uint32` down to 0 underflows the counter, because the default `int` based implementation is applied. - `countdown` on an `uint64` down to 0 with non-1 `step` underflows the counter, because the cancel condition does not consider such steps. - `countdown` on distinct `uint64` down to 0 underflows the counter, because the default implementation is applied. Corrected by replacing the default implementation with the safe `uint` one, and by correcting the cancellation condition for non-1 `step`.
etan-status
force-pushed
the
dev/etan/countdown-uint64
branch
from
d4694ca
to
d7e89cf
Compare
| @@ -38,6 +37,7 @@ iterator countdown*[T](a, b: T, step: Positive = 1): T {.inline.} = | |||
| var res = a | |||
| while res >= b: | |||
| yield res | |||
| if res <= succ(b, step.Natural - 1): break | |||
There was a problem hiding this comment.
Not acceptable. Bloat plus a false sense of "correctness". If you bump into the boundaries of the numeric types that you use, use different numeric types such as a BigInt.
There was a problem hiding this comment.
Any unsigned type stops at 0, regardless of how wide it is. Things like countdown(5.uint64, 0.uint64, 2) won't work.
There was a problem hiding this comment.
re bloat, could you elaborate? The generated code seems to emit a constant for the entire succ(b, step.Natural - 1).
There was a problem hiding this comment.
If you bump into the boundaries of the numeric types that you use, use different numeric types such as a BigInt.
unsigned integers wrap by definition - they're used in contexts where that wrapping is desirable and a different numeric type such as BigInt would simply be wrong - it has a different semantic - the whole point of using unsigned types is that you're looking for a certain behavior at the boundary, and often exploit it - the inability to use for with some uint sizes is a significant gotcha for the language.
|
This pull request is stale because it has been open for 1 year with no activity. Contribute more commits on the pull request and rebase it on the latest devel, or it will be closed in 30 days. Thank you for your contributions. |
github-actions
bot
added
the
stale
|
Bumped to latest |
|
Windows failure seems unrelated: |
github-actions
bot
removed
the
stale
|
I have two other solutions, there is no need to compare more than once in the loop solution 1 when T is (uint|uint64):
if a >= b:
yield a
var res = a
var n = int((a - b) div T(step))
while n > 0:
dec(res, step)
dec(n, 1)
yield ressolution 2 when T is (uint|uint64):
if a >= b:
yield a
var res = a
var nb = b + T((a - b) mod T(step))
while res > nb:
dec(res, step)
yield T(res) |
countdownon anuint32down to 0 underflows the counter, becausethe default
intbased implementation is applied.countdownon anuint64down to 0 with non-1stepunderflows thecounter, because the cancel condition does not consider such steps.
countdownon distinctuint64down to 0 underflows the counter,because the default implementation is applied.
Corrected by replacing the default implementation with the safe
uintone, and by correcting the cancellation condition for non-1
step.