Skip to content

Fix issue - sort should move elements instead of copy... #7524

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
merged 1 commit into from
Jun 19, 2020
Merged

Fix issue - sort should move elements instead of copy... #7524

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
128 changes: 103 additions & 25 deletions std/algorithm/sorting.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1698,17 +1698,30 @@ private void shortSort(alias less, Range)(Range r)
auto t = r[0]; if (pred(t, r[0])) r[0] = r[0];
}))) // Can we afford to temporarily invalidate the array?
{
import core.lifetime : move;

size_t j = i + 1;
auto temp = r[i];
static if (hasLvalueElements!Range)
auto temp = move(r[i]);
else
auto temp = r[i];

if (pred(r[j], temp))
{
do
{
r[j - 1] = r[j];
static if (hasLvalueElements!Range)
trustedMoveEmplace(r[j], r[j - 1]);
else
r[j - 1] = r[j];
++j;
}
while (j < r.length && pred(r[j], temp));
r[j - 1] = temp;

static if (hasLvalueElements!Range)
trustedMoveEmplace(temp, r[j - 1]);
else
r[j - 1] = move(temp);
}
}
else
Expand All @@ -1725,6 +1738,13 @@ private void shortSort(alias less, Range)(Range r)
}
}

/// @trusted wrapper for moveEmplace
private void trustedMoveEmplace(T)(ref T source, ref T target) @trusted
{
import core.lifetime : moveEmplace;
moveEmplace(source, target);
}

@safe unittest
{
import std.random : Random = Xorshift, uniform;
Expand Down Expand Up @@ -2274,9 +2294,9 @@ private template TimSortImpl(alias pred, R)
alias T = ElementType!R;

alias less = binaryFun!pred;
alias greater = (a, b) => less(b, a);
alias greaterEqual = (a, b) => !less(a, b);
alias lessEqual = (a, b) => !less(b, a);
bool greater()(auto ref T a, auto ref T b) { return less(b, a); }
bool greaterEqual()(auto ref T a, auto ref T b) { return !less(a, b); }
bool lessEqual()(auto ref T a, auto ref T b) { return !less(b, a); }
Copy link
Member

Choose a reason for hiding this comment

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

Not introduced here, but it seems these are superfluous (everything can be elegantly expressed with less). greater is not even used.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would agree for greaterEqual(a, b) vs !less(a,b). However, not for lessEqual(a, b) vs !less(b, a). More state for brain to track.


enum minimalMerge = 128;
enum minimalGallop = 7;
Expand Down Expand Up @@ -2448,8 +2468,17 @@ private template TimSortImpl(alias pred, R)
//moveAll(retro(range[lower .. sortedLen]),
// retro(range[lower+1 .. sortedLen+1]));
for (upper=sortedLen; upper > lower; upper--)
range[upper] = range.moveAt(upper - 1);
range[lower] = move(item);
{
static if (hasLvalueElements!R)
move(range[upper -1], range[upper]);
else
range[upper] = range.moveAt(upper - 1);
}

static if (hasLvalueElements!R)
move(item, range[lower]);
else
range[lower] = move(item);
}
}

Expand Down Expand Up @@ -2555,7 +2584,7 @@ private template TimSortImpl(alias pred, R)
copy(range[0 .. mid], temp);

// Move first element into place
range[0] = range[mid];
moveEntry(range, mid, range, 0);

size_t i = 1, lef = 0, rig = mid + 1;
size_t count_lef, count_rig;
Expand All @@ -2572,14 +2601,14 @@ private template TimSortImpl(alias pred, R)
{
if (lessEqual(temp[lef], range[rig]))
{
range[i++] = temp[lef++];
moveEntry(temp, lef++, range, i++);
if (lef >= lef_end) break outer;
++count_lef;
count_rig = 0;
}
else
{
range[i++] = range[rig++];
moveEntry(range, rig++, range, i++);
if (rig >= range.length) break outer;
count_lef = 0;
++count_rig;
Expand All @@ -2590,14 +2619,14 @@ private template TimSortImpl(alias pred, R)
do
{
count_lef = gallopForwardUpper(temp[lef .. $], range[rig]);
foreach (j; 0 .. count_lef) range[i++] = temp[lef++];
foreach (j; 0 .. count_lef) moveEntry(temp, lef++, range, i++);
if (lef >= temp.length) break outer;

count_rig = gallopForwardLower(range[rig .. range.length], temp[lef]);
foreach (j; 0 .. count_rig) range[i++] = range[rig++];
foreach (j; 0 .. count_rig) moveEntry(range, rig++, range, i++);
if (rig >= range.length) while (true)
{
range[i++] = temp[lef++];
moveEntry(temp, lef++, range, i++);
if (lef >= temp.length) break outer;
}

Expand All @@ -2610,11 +2639,11 @@ private template TimSortImpl(alias pred, R)

// Move remaining elements from right
while (rig < range.length)
range[i++] = range[rig++];
moveEntry(range, rig++, range, i++);

// Move remaining elements from left
while (lef < temp.length)
range[i++] = temp[lef++];
moveEntry(temp, lef++, range, i++);

return minGallop > 0 ? minGallop : 1;
}
Expand All @@ -2641,7 +2670,7 @@ private template TimSortImpl(alias pred, R)
copy(range[mid .. range.length], temp);

// Move first element into place
range[range.length - 1] = range[mid - 1];
moveEntry(range, mid - 1, range, range.length - 1);

size_t i = range.length - 2, lef = mid - 2, rig = temp.length - 1;
size_t count_lef, count_rig;
Expand All @@ -2657,19 +2686,19 @@ private template TimSortImpl(alias pred, R)
{
if (greaterEqual(temp[rig], range[lef]))
{
range[i--] = temp[rig];
moveEntry(temp, rig, range, i--);
if (rig == 1)
{
// Move remaining elements from left
while (true)
{
range[i--] = range[lef];
moveEntry(range, lef, range, i--);
if (lef == 0) break;
--lef;
}

// Move last element into place
range[i] = temp[0];
moveEntry(temp, 0, range, i);

break outer;
}
Expand All @@ -2679,10 +2708,10 @@ private template TimSortImpl(alias pred, R)
}
else
{
range[i--] = range[lef];
moveEntry(range, lef, range, i--);
if (lef == 0) while (true)
{
range[i--] = temp[rig];
moveEntry(temp, rig, range, i--);
if (rig == 0) break outer;
--rig;
}
Expand All @@ -2698,18 +2727,18 @@ private template TimSortImpl(alias pred, R)
count_rig = rig - gallopReverseLower(temp[0 .. rig], range[lef]);
foreach (j; 0 .. count_rig)
{
range[i--] = temp[rig];
moveEntry(temp, rig, range, i--);
if (rig == 0) break outer;
--rig;
}

count_lef = lef - gallopReverseUpper(range[0 .. lef], temp[rig]);
foreach (j; 0 .. count_lef)
{
range[i--] = range[lef];
moveEntry(range, lef, range, i--);
if (lef == 0) while (true)
{
range[i--] = temp[rig];
moveEntry(temp, rig, range, i--);
if (rig == 0) break outer;
--rig;
}
Expand Down Expand Up @@ -2806,6 +2835,21 @@ private template TimSortImpl(alias pred, R)
alias gallopForwardUpper = gallopSearch!(false, true);
alias gallopReverseLower = gallopSearch!( true, false);
alias gallopReverseUpper = gallopSearch!( true, true);

/// Helper method that moves from[fIdx] into to[tIdx] if both are lvalues and
/// uses a plain assignment if not (necessary for backwards compatibility)
void moveEntry(X, Y)(ref X from, const size_t fIdx, ref Y to, const size_t tIdx)
{
// This template is instantiated with different combinations of range (R) and temp (T[]).
// T[] obviously has lvalue-elements, so checking R should be enough here
static if (hasLvalueElements!R)
{
import core.lifetime : move;
move(from[fIdx], to[tIdx]);
}
else
to[tIdx] = from[fIdx];
}
}

@safe unittest
Expand Down Expand Up @@ -2918,6 +2962,40 @@ private template TimSortImpl(alias pred, R)
sort!("a < b", SwapStrategy.stable)(arr);
}

@safe unittest
{
static struct NoCopy
{
pure nothrow @nogc @safe:

int key;
this(scope const ref NoCopy)
{
assert(false, "Tried to copy struct!");
}
ref opAssign()(scope const auto ref NoCopy other)
{
assert(false, "Tried to copy struct!");
}
this(this) {}
}

static NoCopy[] makeArray(const size_t size)
{
NoCopy[] array = new NoCopy[](size);
foreach (const i, ref t; array[0..$/2]) t.key = cast(int) (size - i);
foreach (const i, ref t; array[$/2..$]) t.key = cast(int) i;
return array;
}

alias cmp = (ref NoCopy a, ref NoCopy b) => a.key < b.key;
enum minMerge = TimSortImpl!(cmp, NoCopy[]).minimalMerge;

sort!(cmp, SwapStrategy.unstable)(makeArray(20));
sort!(cmp, SwapStrategy.stable)(makeArray(minMerge - 5));
sort!(cmp, SwapStrategy.stable)(makeArray(minMerge + 5));
}

// schwartzSort
/**
Alternative sorting method that should be used when comparing keys involves an
Expand Down