add mapSlice and fix Issue 16501 #4781
Conversation
Current coverage is 88.88% (diff: 92.47%)@@ master #4781 diff @@
==========================================
Files 124 121 -3
Lines 77343 74329 -3014
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
- Hits 68485 66070 -2415
+ Misses 8858 8259 -599
Partials 0 0
|
9il
force-pushed
the
mapSlice
branch
2 times, most recently
from
5191c2e
to
648c25f
Compare
|
Seems you accidentally added internal.o... |
Thanks! |
9il
force-pushed
the
mapSlice
branch
3 times, most recently
from
7f0d1e5
to
9efc78e
Compare
| auto opBinary(string op)(sizediff_t shift) | ||
| if (op == `+` || op == `-`) | ||
| { | ||
| auto ret = this; |
There was a problem hiding this comment.
was auto ret = this.ptrs;.
This is fix for 16501. Unittest can be found below
9il
force-pushed
the
mapSlice
branch
3 times, most recently
from
e6f8290
to
c8fb4fa
Compare
|
@DmitryOlshansky could you please review / merge this PR? It is split into 2 commits. |
9il
force-pushed
the
mapSlice
branch
2 times, most recently
from
89a899f
to
9d6b9a5
Compare
| version(LDC) | ||
| { | ||
| static import ldc.attributes; | ||
| alias fastmath = ldc.attributes.fastmath; |
There was a problem hiding this comment.
We might be a bit careful with this as LDC 1.1 isn't officially released.
@klickverbot is it common to compile a newer Phobos release with an older / stable LDC version?
There was a problem hiding this comment.
No. Phobos is merged with DMD FE
There was a problem hiding this comment.
Furthermore, it is not possible for many releases because new Phobos additions depend on new language, druntime or DMD FE changes.
|
@wilzbach maybe you can review / merge this please? |
|
Thanks! |
| } | ||
|
|
||
| enum FastmathDummy { init } | ||
| FastmathDummy fastmathDummy() { return FastmathDummy.init; } |
There was a problem hiding this comment.
Perhaps nitpicks but:
- Can't
FastmathDummy,fastmathDummyandalias fastmathbe replaced with justenum fastmath; - If
fmbis declared aspackageshould these dummy symbols be that as well, or evenprivate? - BTW, shouldn't all public symbols be
packagein this module, since it's an internal module?
There was a problem hiding this comment.
Can't FastmathDummy, fastmathDummy and alias fastmath be replaced with just enum fastmath;
I am not sure
If fmb is declared as package should these dummy symbols be that as well, or even private?
No
BTW, shouldn't all public symbols be package in this module, since it's an internal module?
No, this module is public because we need 1. Remove existing ndslice modules from Mir. 2. Have additional ndslice modules in Mir
This is part of #4652.
ndMapwas renamed tomapSlice, it is correct, because we already have thisiotaSlice,indexSlice, andrepeatSlice. OtherndXXXalgorithms are not part of this PR.Additional changes:
.slicenow public and moved to.internal. This is done to allow Mir to remove already accepted modules from its code base.PackandMapin internal is stuff that is new (but already reviewed in ndslice.algorithm #4652).The last commit also fix issue 16503.