fix Issue 16315 and optimise arithmetic operations for Slice #4647
Conversation
| slice.popFront; | ||
| rarrary.popFront; | ||
| } | ||
| while (rarrary.length); |
There was a problem hiding this comment.
Isn't
if (a) do
{}
while (a);equivalent to
while(a)
{}There was a problem hiding this comment.
Yes, but the if-do-while
- is common in math software
- is common in other operations in the package, for example for cube it would be
if-if-if-do-do-do-while-while-whilein case of ndslices from both sides.
In addition, compiler may not optimize while to if-do-while.
There was a problem hiding this comment.
It is common, because compilers might fail to optimize it. Many compilers normalize loops to this form. It gives the invariant that the loop body is executed at least once. This gives a place between if and do to move loop invariant things out of the loop, but those things will not get executed if the loop is never entered.
I'm not sure if modern compilers still need this manual optimization. It might just be a habit these days.
There was a problem hiding this comment.
I'm not sure if modern compilers still need this manual optimization. It might just be a habit these days.
Yes, LDC may not optimise it
| /++ | ||
| Assignment of a value of `Slice` type to a $(B fully defined slice). | ||
|
|
There was a problem hiding this comment.
Missing Params section or is it obvious for overloaded operators?
There was a problem hiding this comment.
Yes, the description is part of the spec
|
LGTM |
|
wow yet another sweet PR 👍 |
|
@9il could you rebase this as well please -maybe the increased coverage can help to convince other reviewers ;-) |
|
@wilzbach does the 100 coverave is required ?if there is no public annonce please do not spam prs |
No it's not a requirement yet, but the idea of introducing coverage to Phobos was (1) that new PRs shouldn't unnecessary introduce new misses (and we can slowly cover untested bits) and (2) reviewers can feel a bit safer when all new code is tested. |
Anyway, everyone can see what lines are not covered |
| return opEqualsImpl(this, rslice); | ||
| foreach (i; Iota!(0, PureN)) | ||
| if (this._lengths[i] == 0) | ||
| return true; |
There was a problem hiding this comment.
Looks like more unit tests are needed!
There was a problem hiding this comment.
@WalterBright patch coverage — 96.36% (current Phobos is 88.69). If Phobos is crazy about 100 coverage now, please make an official announce, that NO PRs should be merged with coverage <100%. I don't want to be random victim for coverage idealists.
There was a problem hiding this comment.
All lines should be covered now
|
Auto-merge toggled on |
|
Thanks! |
Current coverage is 88.70% (diff: 100%)@@ master #4647 diff @@
==========================================
Files 121 121
Lines 73855 73916 +61
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 65500 65565 +65
+ Misses 8355 8351 -4
Partials 0 0
|
https://issues.dlang.org/show_bug.cgi?id=16315