D issues are now tracked on GitHub. This Bugzilla instance remains as a read-only archive.
Issue 1861 - (D1 only) .sort fails if opCmp takes a ref param
Summary: (D1 only) .sort fails if opCmp takes a ref param
Status: RESOLVED WORKSFORME
Alias: None
Product: D
Classification: Unclassified
Component: dmd (show other issues)
Version: D1 (retired)
Hardware: x86 Windows
: P2 enhancement
Assignee: Walter Bright
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-02-22 02:15 UTC by Bill Baxter
Modified: 2018-10-22 04:17 UTC (History)
2 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this issue.
Description Bill Baxter 2008-02-22 02:15:37 UTC
------
module opcmpref;
import std.stdio;
struct Pair
{
    int a, b;
    // Sort by b first then a
    //  Sorting fails if rhs is a 'ref' param.  Ok if ref removed.
    int opCmp(ref Pair rhs) {
        if (b!=rhs.b) return b-rhs.b;
        return a-rhs.a;
    }
    string toString()
    {
        return std.string.format("(%s,%s)",a,b);
    }
}
void main()
{
    // Single comparisons are all fine
    assert( !(Pair(0,0)<Pair(0,0)) );
    assert( !(Pair(1,0)<Pair(0,0)) );
    assert( !(Pair(0,1)<Pair(0,0)) );
    assert( !(Pair(1,1)<Pair(0,0)) );
    assert(  (Pair(0,0)<Pair(1,0)) );
    assert( !(Pair(1,0)<Pair(1,0)) );
    assert( !(Pair(0,1)<Pair(1,0)) );
    assert( !(Pair(1,1)<Pair(1,0)) );

    assert(  (Pair(0,0)<Pair(0,1)) );
    assert(  (Pair(1,0)<Pair(0,1)) );
    assert( !(Pair(0,1)<Pair(0,1)) );
    assert( !(Pair(1,1)<Pair(0,1)) );
    assert(  (Pair(0,0)<Pair(1,1)) );
    assert(  (Pair(1,0)<Pair(1,1)) );
    assert(  (Pair(0,1)<Pair(1,1)) );
    assert( !(Pair(1,1)<Pair(1,1)) );

    // But sorting fails when opCmp takes 'ref' param
    auto p = [Pair(0,0), Pair(3,0), Pair(2,1), Pair(1,4)];
    p.sort;
    writefln("p=",p);
    assert(p[0] == Pair(0,0));
    assert(p[1] == Pair(3,0));
    assert(p[2] == Pair(2,1));
    assert(p[3] == Pair(1,4));
}
------------

This may be something in the low-level lib rather than the compiler per-se.  I'm not sure about that.  I have tested and found that the error happens with both Phobos and Tango.
Comment 1 Walter Bright 2008-02-22 14:08:17 UTC
According to http://www.digitalmars.com/d/arrays.html, the parameter to a struct's opCmp() must be S or S*, not ref S.
Comment 2 Andrei Alexandrescu 2008-02-22 14:15:42 UTC
Also, sort in std.algorithm should work with predicates that take references.
Comment 3 Bill Baxter 2008-02-22 16:19:27 UTC
Ok, the documentation says either use Type or Type* for opCmp.  The .sort property does in fact work with either.  The problem with just using Type* is that it isn't called for regular struct comparisons like 
   Type a,b; 
   a<b

The problem with using just Type is that it makes sorting unnecessarily slow, since value arguments will have to be copied for every call of the comparison function.

It makes no sense not to allow ref.  This is exactly the kind of thing ref was meant for, where you want value semantics, but only want to pass a pointer around.

Interestingly, if you provide BOTH a Type* and a ref Type version of opCmp, then regular comparisons will use the ref version, and .sort will use the pointer version.  

But it would be nicer if ref just worked.  I find it very surprising that it doesn't, and until it does I would suggest that the documentation be changed to say in very big bold letters that EVEN THOUGH REF MAY APPEAR TO WORK FOR SMALL TEST CASES IT IS NOT SUPPORTED AND WILL FAIL IF YOU TRY TO .sort.
Comment 4 Yao Gomez 2012-02-05 01:01:46 UTC
Marking this as D1 only, as the built-in, AA.sort method is going to be deprecated.

With a slight modification, your test example passes and prints the correct result with the std.algorithm.sort function (D2 only).

------
module opcmpref;

import std.stdio, std.algorithm;

struct Pair
{
    int a, b;
    // Sort by b first then a
    //  Sorting fails if rhs is a 'ref' param.  Ok if ref removed.
    int opCmp(ref Pair rhs) {
        if (b!=rhs.b) return b-rhs.b;
        return a-rhs.a;
    }
    @property string toString()
    {
        return std.string.format("(%s,%s)",a,b);
    }
}
void main()
{
    // Single comparisons are all fine
    assert( !(Pair(0,0)<Pair(0,0)) );
    assert( !(Pair(1,0)<Pair(0,0)) );
    assert( !(Pair(0,1)<Pair(0,0)) );
    assert( !(Pair(1,1)<Pair(0,0)) );
    assert(  (Pair(0,0)<Pair(1,0)) );
    assert( !(Pair(1,0)<Pair(1,0)) );
    assert( !(Pair(0,1)<Pair(1,0)) );
    assert( !(Pair(1,1)<Pair(1,0)) );

    assert(  (Pair(0,0)<Pair(0,1)) );
    assert(  (Pair(1,0)<Pair(0,1)) );
    assert( !(Pair(0,1)<Pair(0,1)) );
    assert( !(Pair(1,1)<Pair(0,1)) );
    assert(  (Pair(0,0)<Pair(1,1)) );
    assert(  (Pair(1,0)<Pair(1,1)) );
    assert(  (Pair(0,1)<Pair(1,1)) );
    assert( !(Pair(1,1)<Pair(1,1)) );

    auto p = [Pair(0,0), Pair(3,0), Pair(2,1), Pair(1,4)];
    std.algorithm.sort(p);
    // prints 'p=[(0,0), (3,0), (2,1), (1,4)]'
    writefln("p=%s",p);

    assert(p[0] == Pair(0,0));
    assert(p[1] == Pair(3,0));
    assert(p[2] == Pair(2,1));
    assert(p[3] == Pair(1,4));
}
------
Comment 5 Kenji Hara 2013-11-24 19:32:09 UTC
D1 is not supported anymore, and in D2 built-in sort property will be deprecated.
Comment 6 Mathias LANG 2018-10-22 04:17:14 UTC
D2 builtin sort is gone, closing.