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 issue 16246 - cannot call iota with 3 [u]bytes or 3 [u]shorts #5391
Conversation
|
std/range/package.d
Outdated
@@ -5747,6 +5751,18 @@ debug @system unittest | |||
} | |||
} | |||
|
|||
@safe @nogc nothrow unittest |
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.
pure
?
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.
ok
std/range/package.d
Outdated
{ | ||
{ | ||
ushort start = 0, end = 10, step = 2; | ||
foreach (i; iota(start, end, step)) {} |
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.
It may be useful to statically assert that i
is of the expected type -ushort
here and ubyte
below.
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.
or, just foreach(ushort i; ...)
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.
ok
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.
Approved, but see comments for optional improvements.
@@ -5209,6 +5209,7 @@ auto sequence(alias fun, State...)(State args) | |||
assert(s.front != s.front); // no caching | |||
} | |||
|
|||
// iota |
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.
Why this stray comment?
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.
Helps quickly locating the start of the implementation.
std/range/package.d
Outdated
assert(unsigned((current - pastLast) / -step) <= size_t.max); | ||
|
||
this.pastLast = pastLast + 1; | ||
assert(step == 0 || unsigned((current - pastLast) / -step) <= size_t.max); |
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.
step cannot be 0 here.
assert(step != 0 && ...)
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.
Or, you can just remove that part of the assert.
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.
ok
std/range/package.d
Outdated
|
||
this.pastLast = pastLast - 1; | ||
// Cast below can't fail because current < pastLast | ||
this.pastLast = cast(Value) (pastLast - 1); |
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 logic is correct, but hard to follow.
I'm wondering if it would be better to refactor this into three if branches:
if(current < pastLast && step > 0)
{
this.step = step;
this.current = current;
... // code currently inside the step > 0 branch
this.pastLast += step; // this could probably be folded into last line
}
else if(current > pastLast && step < 0)
{
this.step = step;
this.current = current;
... // code currently inside the else branch of the inner if block
this.pastLast += step;
}
else
{
... // step is 0 or current is on wrong side of pastLast
}
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.
It's a a toss seeing that this variant has more duplication. I'll try a variant with early returns.
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.
It's a a toss seeing that this variant has more duplication.
True, but not duplication of conditionals that have already been checked :) There is also the inferred logic in your comment (if step > 0, then current must be < lastPast due to how the big if condition is constructed) that becomes clearer when the actual if condition is simplified.
This seems to me like the debate about combining versions and repeating the declarations. One is more DRY, but the other makes it clear which declarations are active for a specific version.
In any case, I'm OK with the code as written as it is correct and not really more confusing than the original, it just was hard to follow.
std/range/package.d
Outdated
{ | ||
{ | ||
ushort start = 0, end = 10, step = 2; | ||
foreach (i; iota(start, end, step)) {} |
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.
or, just foreach(ushort i; ...)
Had to overhaul things a bit, representing the limit as "last" instead of "past last" because of overflows. |
Excellent, looks MUCH better and easier to follow the logic! Should be good to merge. Using |
Another nice thing to do with iota is have it accept a compile-time step. I.e. the |
not much to say