[Static if] replace overload constraints with static if (sorting.d)#5151
[Static if] replace overload constraints with static if (sorting.d)#5151dlang-bot merged 1 commit intodlang:masterfrom
Conversation
wilzbach
left a comment
wilzbach
left a comment
There was a problem hiding this comment.
Notice that the first three constraints are exactly the same and only the last one differs.
std/algorithm/sorting.d
Outdated
| isRandomAccessRange!RangeIndex && | ||
| hasAssignableElements!RangeIndex && | ||
| isIntegral!(ElementType!(RangeIndex))) | ||
| isIntegral!(ElementType!(RangeIndex)) || is(ElementType!(RangeIndex) == ElementType!(Range)*)) |
There was a problem hiding this comment.
The next, to be grouped overload is
isRandomAccessRange!Range &&
isRandomAccessRange!RangeIndex &&
hasAssignableElements!RangeIndex &&
is(ElementType!(RangeIndex) == ElementType!(Range)*))Notice that the first three constraints are exactly equal and only the last one differs as the first overload is for a range of indexes and the second one for a range of pointers.
wilzbach
force-pushed
the
static_if_sorting
branch
from
c652bce to
d4b1b2e
Compare
std/algorithm/sorting.d
Outdated
| isRandomAccessRange!RangeIndex && | ||
| hasAssignableElements!RangeIndex && | ||
| isIntegral!(ElementType!(RangeIndex))) | ||
| isIntegral!(ElementType!(RangeIndex)) || is(ElementType!(RangeIndex) == ElementType!(Range)*)) |
There was a problem hiding this comment.
What's with the extra 4 leading spaces?
I suggest adding parens around this line to make the logic grouping clearer.
std/algorithm/sorting.d
Outdated
| } | ||
| auto heap = BinaryHeap!(RangeIndex, indirectLess)(index, 0); | ||
| foreach (i; 0 .. r.length) | ||
| else |
There was a problem hiding this comment.
Might be good to actually have a static if here as well and an else static assert(false) at the end to help ensure that the constraint and static ifs stay in sync. (E.g. if someone introduces a bug and a user finds it in a random corner case, they'll get a static assertion instead of potentially wrong execution or some compiler error inside phobos)
|
Other than the minor comments above, LGTM |
|
ping @wilzbach Let's address @John-Colvin 's comments and let's get this PR merged. It's been sitting here for far too long already. |
wilzbach
force-pushed
the
static_if_sorting
branch
from
d4b1b2e to
59508c4
Compare
| isRandomAccessRange!RangeIndex && | ||
| hasAssignableElements!RangeIndex && | ||
| isIntegral!(ElementType!(RangeIndex))) | ||
| hasAssignableElements!RangeIndex) |
There was a problem hiding this comment.
As suggested I moved the specific ElementType checks and trigger a static assert error (and thus nice error message!) if an invalid type is passed by accident.
I absolutely agree & I am quite sorry that I have been so busy over the last weeks. |
|
LGTM |
|
@wilzbach you're on fire over there |
Isn't most of the Phobos queue on 🔥? |
5 participants
Split up of #5145