www.digitalmars.com         C & C++   DMDScript  

digitalmars.D.bugs - [Issue 1861] New: .sort fails if opCmp takes ref param

reply d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=1861

           Summary: .sort fails if opCmp takes ref param
           Product: D
           Version: 1.027
          Platform: PC
        OS/Version: Windows
            Status: NEW
          Severity: major
          Priority: P2
         Component: DMD
        AssignedTo: bugzilla digitalmars.com
        ReportedBy: wbaxter gmail.com


------
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.


-- 
Feb 22 2008
next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=1861


bugzilla digitalmars.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|                            |INVALID





According to http://www.digitalmars.com/d/arrays.html, the parameter to a
struct's opCmp() must be S or S*, not ref S.


-- 
Feb 22 2008
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=1861






Also, sort in std.algorithm should work with predicates that take references.


-- 
Feb 22 2008
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=1861


wbaxter gmail.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Severity|major                       |enhancement
             Status|RESOLVED                    |REOPENED
         Resolution|INVALID                     |
            Summary|.sort fails if opCmp takes  |make .sort not fail if opCmp
                   |ref param                   |takes ref param





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.


-- 
Feb 22 2008
prev sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=1861


Yao Gomez <yao.gomez gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |yao.gomez gmail.com
            Version|1.027                       |D1
            Summary|.sort fails if opCmp takes  |(D1 only) .sort fails if
                   |a ref param                 |opCmp takes a ref param



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));
}
------

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Feb 05 2012