Unify function overloads in std.range#5605
Conversation
wilzbach
requested review from
JackStouffer,
PetarKirov and
andralex
as code owners
|
Thanks for your pull request, @wilzbach! Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. |
quickfur
left a comment
quickfur
left a comment
There was a problem hiding this comment.
Overall, LGTM, other than my question about using Unqual in chooseAmong.
| if (Ranges.length > 2 | ||
| && is(typeof(choose(true, rs[0], rs[1]))) | ||
| && is(typeof(chooseAmong(0, rs[1 .. $])))) | ||
| if (Ranges.length >= 2 && allSatisfy!(isInputRange, staticMap!(Unqual, Ranges))) |
There was a problem hiding this comment.
Why Unqual here? Does chooseAmong actually work for, say, immutable "ranges"?
There was a problem hiding this comment.
Because of the is(typeof(choose(true, rs[0], rs[1]))) constraint, which reduces to the following:
auto choose(R1, R2)(bool condition, R1 r1, R2 r2)
if (isInputRange!(Unqual!R1) && isInputRange!(Unqual!R2) &&
!is(CommonType!(ElementType!(Unqual!R1), ElementType!(Unqual!R2)) == void))(The common type is handles latter as static assert for user-friendliness)
There was a problem hiding this comment.
Yep, but I didn't want to touch the behavior in this PR - so I opted to emulate the current behavior.
| { | ||
| static assert(0, "The ranges have no common type."); | ||
| } | ||
| } |
There was a problem hiding this comment.
This merged overload set LGTM, other than my question about Unqual.
| else | ||
| { | ||
| return Take!R(input, n); | ||
| } |
There was a problem hiding this comment.
This merged overload set also LGTM.
| static if (!is(R == class)) | ||
| return RefRange!R(range); | ||
| else | ||
| return *range; |
There was a problem hiding this comment.
This merged overload set also LGTM. Thanks!
|
@wilzbach can you put each overload set change into a separate commit? |
|
@ZombineDev Good idea! |
PetarKirov
left a comment
PetarKirov
left a comment
There was a problem hiding this comment.
LGTM, just move the changes into separate commits.
wilzbach
force-pushed
the
unify-std-range
branch
from
649d9a8 to
14650a5
Compare
Okay - good suggestion! |
|
@CyberShadow did I break DAutoTest? ;-) |
|
Heh, that's a race condition. It'll get round to retest it |
I can simply rebase, I just thought I let you know while it's still easy to see the logs ;-) |
wilzbach
force-pushed
the
unify-std-range
branch
from
14650a5 to
e7a1c94
Compare
|
Yep, not a bug. IIRC Travis will also fail when you update a PR while it's starting up. |
|
Hmm it happened again - even with my rebase:
|
wilzbach
force-pushed
the
unify-std-range
branch
from
e7a1c94 to
f13a282
Compare
|
Okay, as rebasing doesn't seem to help, I will try to open a new PR :/ |
|
Ping me if it happens again, I'll see which SHA is wrong. |
|
Actually let me add that to the error message right now. |
While reopening fixed the problem with CircleCi, it seems like DAutoTest still doesn't like it. |
Closing and reopening has zero effect on DAutoTest currently, you'll need to rebase or wait until something else is merged and it tests the PR again against the new merge target. |
5 participants
Yet another small step towards better, more digestable documentation (and hiding things that the user doesn't need to know).
Let's see whether the CODEOWNERS file finally works ...