Skip to content

Fix Issue 5224 - std.algorithm.remove!(SwapStrategy.unstable) doesn't work #1162

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.

Sign up for GitHub

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

Merged
4 commits merged into from
Jul 30, 2013
Merged

Fix Issue 5224 - std.algorithm.remove!(SwapStrategy.unstable) doesn't work #1162

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
98b47ad
issue 5224
andralex Feb 20, 2013
ac555de
Merge branch 'master' of github.com:D-Programming-Language/phobos int…
andralex May 26, 2013
bff9bf2
@blackwhale's review
andralex May 26, 2013
ae8f014
@blackwhale's destruction
andralex May 27, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
170 changes: 79 additions & 91 deletions std/algorithm.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7267,120 +7267,80 @@ cases.))
Range remove
(SwapStrategy s = SwapStrategy.stable, Range, Offset...)
(Range range, Offset offset)
if (isBidirectionalRange!Range && hasLength!Range && s != SwapStrategy.stable
if (s != SwapStrategy.stable
&& isBidirectionalRange!Range && hasLength!Range
&& Offset.length >= 1)
{
enum bool tupleLeft = is(typeof(offset[0][0]))
&& is(typeof(offset[0][1]));
enum bool tupleRight = is(typeof(offset[$ - 1][0]))
&& is(typeof(offset[$ - 1][1]));
static if (!tupleLeft)
Tuple!(size_t, "pos", size_t, "len")[offset.length] blackouts;
foreach (i, v; offset)
{
alias offset[0] lStart;
auto lEnd = lStart + 1;
}
else
{
auto lStart = offset[0][0];
auto lEnd = offset[0][1];
}
static if (!tupleRight)
{
alias offset[$ - 1] rStart;
auto rEnd = rStart + 1;
}
else
{
auto rStart = offset[$ - 1][0];
auto rEnd = offset[$ - 1][1];
}
// Begin. Test first to see if we need to remove the rightmost
// element(s) in the range. In that case, life is simple - chop
// and recurse.
if (rEnd == range.length)
{
// must remove the last elements of the range
range.popBackN(rEnd - rStart);
static if (Offset.length > 1)
static if (is(typeof(v[0]) : size_t) && is(typeof(v[1]) : size_t))
{
return .remove!(s, Range, Offset[0 .. $ - 1])
(range, offset[0 .. $ - 1]);
blackouts[i].pos = v[0];
blackouts[i].len = v[1] - v[0];
}
else
{
return range;
}
}

// Ok, there are "live" elements at the end of the range
auto t = range;
auto lDelta = lEnd - lStart, rDelta = rEnd - rStart;
auto rid = min(lDelta, rDelta);
foreach (i; 0 .. rid)
{
move(range.back, t.front);
range.popBack();
t.popFront();
}
if (rEnd - rStart == lEnd - lStart)
{
// We got rid of both left and right
static if (Offset.length > 2)
{
return .remove!(s, Range, Offset[1 .. $ - 1])
(range, offset[1 .. $ - 1]);
static assert(is(typeof(v) : size_t), typeof(v).stringof);
blackouts[i].pos = v;
blackouts[i].len = 1;
}
else
{
return range;
static if (i > 0)
{
enforce(blackouts[i - 1].pos + blackouts[i - 1].len
<= blackouts[i].pos,
"remove(): incorrect ordering of elements to remove");
}
}
else if (rEnd - rStart < lEnd - lStart)

size_t left = 0, right = offset.length - 1;
auto tgt = range.save;
size_t steps = 0;

while (left <= right)
{
// We got rid of the entire right subrange
static if (Offset.length > 2)
{
return .remove!(s, Range)
(range, tuple(lStart + rid, lEnd),
offset[1 .. $ - 1]);
}
else
// Look for a blackout on the right
if (blackouts[right].pos + blackouts[right].len >= range.length)
{
auto tmp = tuple(lStart + rid, lEnd);
return .remove!(s, Range, typeof(tmp))
(range, tmp);
range.popBackN(blackouts[right].len);
--right;
continue;
}
}
else
{
// We got rid of the entire left subrange
static if (Offset.length > 2)
// Advance to next blackout on the left
assert(blackouts[left].pos >= steps);
tgt.popFrontN(blackouts[left].pos - steps);
steps = blackouts[left].pos;
auto toMove = min(
blackouts[left].len,
range.length - (blackouts[right].pos + blackouts[right].len));
foreach (i; 0 .. toMove)
{
return .remove!(s, Range)
(range, offset[1 .. $ - 1],
tuple(rStart, lEnd - rid));
move(range.back, tgt.front);
range.popBack();
tgt.popFront();
}
else
steps += toMove;
if (toMove == blackouts[left].len)
{
auto tmp = tuple(rStart, lEnd - rid);
return .remove!(s, Range, typeof(tmp))
(range, tmp);
// Filled the entire left hole
++left;
continue;
}
}

return range;
}

// Ditto
Range remove
(SwapStrategy s = SwapStrategy.stable, Range, Offset...)
(Range range, Offset offset)
if ((isForwardRange!Range && !isBidirectionalRange!Range
|| !hasLength!Range || s == SwapStrategy.stable)
&& Offset.length >= 1)
if (s == SwapStrategy.stable && isForwardRange!Range && Offset.length >= 1)
{
auto result = range;
auto src = range, tgt = range;
size_t pos;
foreach (i; offset)
foreach (pass, i; offset)
{
static if (is(typeof(i[0])) && is(typeof(i[1])))
{
Expand All @@ -7391,10 +7351,20 @@ if ((isForwardRange!Range && !isBidirectionalRange!Range
auto from = i;
enum delta = 1;
}
assert(pos <= from);
for (; pos < from; ++pos, src.popFront(), tgt.popFront())
enforce(pos <= from,
"remove(): incorrect ordering of elements to remove");
if (pass > 0)
{
move(src.front, tgt.front);
for (; pos < from; ++pos, src.popFront(), tgt.popFront())
{
move(src.front, tgt.front);
}
}
else
{
src.popFrontN(from);
tgt.popFrontN(from);
pos = from;
}
// now skip source to the "to" position
src.popFrontN(delta);
Expand All @@ -7406,6 +7376,16 @@ if ((isForwardRange!Range && !isBidirectionalRange!Range
return result;
}

unittest
{
// http://d.puremagic.com/issues/show_bug.cgi?id=10173
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The last thing is that I'm not quite sure what is the resolution of this bug then?
a) documentation is wrong, it should say assert (as tested below)
b) implementation is wrong, it should do enforce/throw

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, I thought assertThrown means "assert an exception was thrown". Instead it seems to mean "make sure that an assert was thrown". Do we have an assertException or something?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mm shame on me. Carry on.
It does check that something throws a derivative of Exception.
And an assertion failure is caught by
assertThrown!AssertionError(....);

int[] test = iota(0, 10).array();
assertThrown(remove!(SwapStrategy.stable)(test, tuple(2, 4), tuple(1, 3)));
assertThrown(remove!(SwapStrategy.unstable)(test, tuple(2, 4), tuple(1, 3)));
assertThrown(remove!(SwapStrategy.stable)(test, 2, 4, 1, 3));
assertThrown(remove!(SwapStrategy.unstable)(test, 2, 4, 1, 3));
}

unittest
{
debug(std_algorithm) scope(success)
Expand All @@ -7418,11 +7398,15 @@ unittest

a = [ 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ];
assert(remove!(SwapStrategy.unstable)(a, 0, 10) ==
[ 9, 1, 2, 3, 4, 5, 6, 7, 8 ]);
[ 9, 1, 2, 3, 4, 5, 6, 7, 8 ]);

a = [ 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ];
assert(remove!(SwapStrategy.unstable)(a, 0, tuple(9, 11)) ==
[ 8, 1, 2, 3, 4, 5, 6, 7 ]);
// http://d.puremagic.com/issues/show_bug.cgi?id=5224
a = [ 1, 2, 3, 4 ];
assert(remove!(SwapStrategy.unstable)(a, 2) ==
[ 1, 2, 4 ]);

a = [ 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ];
//writeln(remove!(SwapStrategy.stable)(a, 1, 5));
Expand All @@ -7440,6 +7424,10 @@ unittest
a = [ 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ];
assert(remove!(SwapStrategy.stable)(a, 1, tuple(3, 5))
== [ 0, 2, 5, 6, 7, 8, 9, 10]);

a = iota(0, 10).array();
assert(remove!(SwapStrategy.unstable)(a, tuple(1, 4), tuple(6, 7))
== [0, 9, 8, 7, 4, 5]);
}

/**
Expand All @@ -7465,7 +7453,7 @@ if (isBidirectionalRange!Range)
{
for (;!range.empty;)
{
if (!unaryFun!(pred)(range.front))
if (!unaryFun!pred(range.front))
{
range.popFront();
continue;
Expand Down