www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - Issue 3789, stucts equality

reply bearophile <bearophileHUGS lycos.com> writes:
Issue 3789 is an enhancement request, I think it fixes one small but quite
important problem in D design. The situation is shown by this simple code:


struct String {
    char[] data;
}
void main () {
    auto foo = String("foo".dup);
    auto bar = String("foo".dup);
    assert(bar !is foo, "structs aren't the same bit-wise");
    assert(bar == foo, "oops structs aren't equal");
}



The D Zen says D is designed to be safe on default and to perform unsafe (and
faster) things on request. Not comparing the strings as strings in the
following code breaks the Principle of least astonishment, so breaks that rule.

An acceptable alternative to fixing Bug 3789 is statically disallowing the
equal operator (==) in such cases (or even in all cases).

D has the "is" for the situations where you want to perform bitwise comparison,
for structs too. For the other situations where I use "==" among struts, I want
it do the right thing, like comparing its contained strings correctly instead
of arbitrarily deciding to use bitwise comparison of the sub-struct that
represents the string.

There is already a patch for this, from the extra-good Kenji Hara:
https://github.com/D-Programming-Language/dmd/pull/387

Making "==" work as "is" for structs means using an operator for the purpose of
the other operator, and it has caused some bugs in my code. And it will cause
bugs in D code to come.


Another example, reduced/modified from a real bug in a program of mine:


import std.stdio;
struct Foo {
    int x;
    string s;
}
void main () {
    int[Foo] aa;
    aa[Foo(10, "hello")] = 1;
    string hel = "hel";
    aa[Foo(10, hel ~ "lo")] = 2;
    writeln(aa);
}


Here D defines a hashing for the Foo struct, but it uses the standard "==" to
compare the struct keys. So the output is this, that I believe is what almost
no one will ever want:

[Foo(10, "hello"):1, Foo(10, "hello"):2]

Bye,
bearophile
Mar 26 2012
parent reply Alvaro <alvaroDotSegura gmail.com> writes:
El 26/03/2012 13:58, bearophile escribió:

 Issue 3789 is an enhancement request, I think it fixes one small but quite
important problem in D design. The situation is shown by this simple code:


 struct String {
      char[] data;
 }
 void main () {
      auto foo = String("foo".dup);
      auto bar = String("foo".dup);
      assert(bar !is foo, "structs aren't the same bit-wise");
      assert(bar == foo, "oops structs aren't equal");
 }



 The D Zen says D is designed to be safe on default and to perform unsafe (and
faster) things on request. Not comparing the strings as strings in the
following code breaks the Principle of least astonishment, so breaks that rule.



Maybe it makes more sense that struct==struct applies == to each of its 
fields. It would be the same as bitwise comparison for simple primitive 
types, but would be more useful with other types such as strings.
Mar 26 2012
next sibling parent "Nathan M. Swan" <nathanmswan gmail.com> writes:
On Monday, 26 March 2012 at 21:25:06 UTC, Alvaro wrote:

 Maybe it makes more sense that struct==struct applies == to 
 each of its fields. It would be the same as bitwise comparison 
 for simple primitive types, but would be more useful with other 
 types such as strings.


+1
Mar 27 2012
prev sibling parent reply "Steven Schveighoffer" <schveiguy yahoo.com> writes:
On Mon, 26 Mar 2012 17:25:07 -0400, Alvaro <alvaroDotSegura gmail.com>
wrote:


 Maybe it makes more sense that struct==struct applies == to each of its  
 fields. It would be the same as bitwise comparison for simple primitive  
 types, but would be more useful with other types such as strings.


That's exactly what bug 3789 advocates for.  Please vote up!

http://d.puremagic.com/issues/show_bug.cgi?id=3789

-Steve
Mar 27 2012
next sibling parent reply Andrej Mitrovic <andrej.mitrovich gmail.com> writes:
On 3/27/12, Steven Schveighoffer <schveiguy yahoo.com> wrote:

 That's exactly what bug 3789 advocates for.  Please vote up!

 http://d.puremagic.com/issues/show_bug.cgi?id=3789


Unrelated, but I don't understand bugzilla's stupid 10-vote
limitation. As if I can only care about 10 bugs. I agree with 3789, ==
should do the safe thing by default.

Speaking of which, does anyone have some sort of template to
autogenerate an opEquals that compares all fields of a struct via
'=='? I could write it (maybe via traits & getAllMembers) but surely
someone has done it before.
Mar 29 2012
parent "Steven Schveighoffer" <schveiguy yahoo.com> writes:
On Thu, 29 Mar 2012 07:49:28 -0400, Andrej Mitrovic  
<andrej.mitrovich gmail.com> wrote:


 On 3/27/12, Steven Schveighoffer <schveiguy yahoo.com> wrote:

 That's exactly what bug 3789 advocates for.  Please vote up!

 http://d.puremagic.com/issues/show_bug.cgi?id=3789


 Unrelated, but I don't understand bugzilla's stupid 10-vote
 limitation. As if I can only care about 10 bugs. I agree with 3789, ==
 should do the safe thing by default.


The reasoning behind it was that we didn't want everyone voting for every  
bug.  10 votes may be too restrictive, but some restriction should be in  
place IMO.  The 10 votes was an arbitrary choice.  When you have unlimited  
votes, you are not as responsible with them.

I personally have never run out of votes, because every time I go to vote  
for a bug, I have some voted-for bugs which have been fixed.

The voting system was put in place a few years ago, to see how it might  
help focus attention on more severe bugs.  I'm not sure it had the  
dramatic effect we were hoping, but some bugs did get more attention  
because they had more votes.  It at least gives you a good compelling  
argument if you want to argue that a high-vote bug should be fixed :)

-Steve
Mar 29 2012
prev sibling next sibling parent Andrej Mitrovic <andrej.mitrovich gmail.com> writes:
On 3/29/12, Andrej Mitrovic <andrej.mitrovich gmail.com> wrote:

 Speaking of which, does anyone have some sort of template to
 autogenerate an opEquals that compares all fields of a struct via
 '=='?


Well that took 2 seconds.

import std.range;

template safeOpEquals(T)
{
    bool opEquals(T t)
    {
        foreach (lhsField, rhsField; lockstep(this.tupleof, t.tupleof))
        {
            if (lhsField != rhsField)
                return false;
        }

        return true;
    }
}

struct Foo
{
    int[] arr;
    mixin safeOpEquals!Foo;
}

void main()
{
    Foo a, b;
    a.arr = [1, 2, 3].dup;
    b.arr = [1, 2, 3].dup;
    assert(a == b);
}

D makes things so damn easy.
Mar 29 2012
prev sibling next sibling parent Andrej Mitrovic <andrej.mitrovich gmail.com> writes:
On 3/29/12, Andrej Mitrovic <andrej.mitrovich gmail.com> wrote:

 D makes things so damn easy.


Correction, makes things easier. No need to pass the type of 'this':

template safeOpEquals()
{
    bool opEquals(typeof(this) t)
    {
        foreach (lhsField, rhsField; lockstep(this.tupleof, t.tupleof)) {
            if (lhsField != rhsField)
                return false;
        }
        return true;
    }
}
struct Foo
{
    int[] arr;
    mixin safeOpEquals;
}
Mar 29 2012
prev sibling next sibling parent Andrej Mitrovic <andrej.mitrovich gmail.com> writes:
 3/29/12, Andrej Mitrovic <andrej.mitrovich gmail.com> wrote:

 snip


Crap, lockstep doesn't work on different types, it was iterating over
the arrays. That went over my head. A slightly complicated version
that actually works:

import std.stdio;
import std.conv;

string tupleCompare(int len)()
{
    string result;
    foreach (i; 0 .. len)
    {
        result ~= "
            if (this.tupleof[" ~ to!string(i) ~ "] != t.tupleof[" ~
to!string(i) ~ "])
                return false;
        ";
    }
    return result;
}

template safeOpEquals()
{
    bool opEquals(typeof(this) t)
    {
        enum len = typeof(this).tupleof.length;
        mixin(tupleCompare!len);
        return true;
    }
}
Mar 29 2012
prev sibling next sibling parent Artur Skawina <art.08.09 gmail.com> writes:
On 03/29/12 14:07, Andrej Mitrovic wrote:

  3/29/12, Andrej Mitrovic <andrej.mitrovich gmail.com> wrote:

 snip


 
 Crap, lockstep doesn't work on different types, it was iterating over
 the arrays. That went over my head. A slightly complicated version
 that actually works:
 
 import std.stdio;
 import std.conv;
 
 string tupleCompare(int len)()
 {
     string result;
     foreach (i; 0 .. len)
     {
         result ~= "
             if (this.tupleof[" ~ to!string(i) ~ "] != t.tupleof[" ~
 to!string(i) ~ "])
                 return false;
         ";
     }
     return result;
 }
 
 template safeOpEquals()
 {
     bool opEquals(typeof(this) t)
     {
         enum len = typeof(this).tupleof.length;
         mixin(tupleCompare!len);
         return true;
     }
 }


// D does make things easy:
template safeOpEquals()
{
   bool opEquals(T)(T t) {
      foreach (i, v; this.tupleof )
         if (v != t.tupleof[i])
            return 0;
      return 1;
   }
}

struct Foo
{
    int[] arr;
    string s;
    mixin safeOpEquals;
}


You may also want to include something like


   static if (!is(typeof(this)==T))
      return 0;
   else
      foreach etc...


to make it return false, instead of failing at compiletime when
comparing different types.

artur
Mar 29 2012
prev sibling parent reply Alvaro <alvaroDotSegura gmail.com> writes:
El 27/03/2012 22:20, Steven Schveighoffer escribió:

 On Mon, 26 Mar 2012 17:25:07 -0400, Alvaro <alvaroDotSegura gmail.com>
 wrote:


 Maybe it makes more sense that struct==struct applies == to each of
 its fields. It would be the same as bitwise comparison for simple
 primitive types, but would be more useful with other types such as
 strings.


 That's exactly what bug 3789 advocates for. Please vote up!

 http://d.puremagic.com/issues/show_bug.cgi?id=3789

 -Steve


BTW, today I encountered a problem that is probably related to this bug.

if you have a map with BigInt as key, it duplicates keys:

int[BigInt] a;
a[BigInt(3)] = 1;
a[BigInt(3)] = 2;
writeln(a);

prints

[3:2, 3:1]
Mar 29 2012
parent bearophile <bearophileHUGS lycos.com> writes:
Alvaro:


 BTW, today I encountered a problem that is probably related to this bug.
 
 if you have a map with BigInt as key, it duplicates keys:
 
 int[BigInt] a;
 a[BigInt(3)] = 1;
 a[BigInt(3)] = 2;
 writeln(a);
 
 prints
 
 [3:2, 3:1]


Thank you for your example.

BigInt usage has several other problems, this is only a partial sample of them:

-----------------------------

import std.bigint;
void main() {
    BigInt[10] a;
    a[0] = 1; // OK
    a[] = 1; // Error
}


test.d(5): Error: cannot implicitly convert expression (1) of type int to
BigInt[]

-----------------------------

import std.bigint;
void main() {
    BigInt b;
    switch (b) {
        case BigInt(0): break;
        default: break;
    }
}


test2.d(4): Error: 'b' is not of integral type, it is a BigInt
test2.d(5): Error: case must be a string or an integral constant, not
BigInt(BigUint([0u]),false)

-----------------------------

import std.bigint;
struct Count(T) {
    T n;
    this(T n_) { this.n = n_; }
    const bool empty = false;
     property T front() { return n; }
    void popFront() { n++; } // line 7
}
void main() {
    auto co = Count!BigInt(BigInt(0));
    foreach (b; co) {}
}


test2.d(7): Error: var has no effect in expression (__pitmp877)
test2.d(10): Error: template instance test.Count!(BigInt) error instantiating

-----------------------------

You can't compile this program:


import std.stdio, std.bigint;

BigInt positiveIntegerPow(in BigInt inBase, in BigInt inExp) pure nothrow
in {
    assert(inBase >= 0 && inExp >= 0);
} out(result) {
    assert(result >= 0);
} body {
    BigInt base = inBase;
    BigInt exp = inExp;
    auto result = BigInt(1);
    while (exp) {
        if (exp & 1)
            result *= base;
        exp >>= 1;
        base *= base;
    }

    return result;
}

void main() {
    writeln(positiveIntegerPow(BigInt(5), BigInt(6)));
}


You have to change it to:


import std.stdio, std.bigint;

BigInt positiveIntegerPow(in BigInt inBase, in BigInt inExp)
in {
    assert(cast()inBase >= 0 && cast()inExp >= 0);
} out(result) {
    assert(cast()result >= 0);
} body {
    BigInt base = cast()inBase;
    BigInt exp = cast()inExp;
    auto result = BigInt(1);
    while (exp != 0) {
        if (exp % 2)
            result *= base;
        exp >>= 1;
        base *= base;
    }

    return result;
}

void main() {
    writeln(positiveIntegerPow(BigInt(5), BigInt(6)));
}


The problems in that little program are:
- You can't assign a const BigInt to a nonconst one.
- BigInt operations aren't pure or nothrow
- while(exp) is not supported
- if if(exp & 1) is not supported
- assert(result>=0); where result is const is not supported.

Bye,
bearophile
Mar 29 2012