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

           Summary: [tdpl] Comparing structs for equality is not
                    member-by-member
           Product: D
           Version: unspecified
          Platform: Other
        OS/Version: Linux
            Status: NEW
          Severity: normal
          Priority: P2
         Component: DMD
        AssignedTo: nobody@puremagic.com
        ReportedBy: andrei@metalanguage.com


--- Comment #0 from Andrei Alexandrescu <andrei@metalanguage.com> 2009-10-21 07:40:35 PDT ---
import std.math, std.stdio;

struct Point {
   float x = 0, y = 0;
   // /*[\textit{\textbf{Added}}]*/
   bool opEquals(Point rhs) {
      // Perform an approximate comparison
      return approxEqual(x, rhs.x, 1e-3, 1e-5) && approxEqual(y, rhs.y, 1e-3,
1e-5);
   }
}

struct Rectangle {
   Point leftBottom, rightTop;
}

unittest {
   Rectangle a, b;
   assert(a == b);
   a.leftBottom.x = 1e-8;
   assert(a == b);
   a.rightTop.y = 5;
   assert(a != b);
}

This fails because Point.opEquals is never called.

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

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


Ary Borenszweig <ary@esperanto.org.ar> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ary@esperanto.org.ar


--- Comment #1 from Ary Borenszweig <ary@esperanto.org.ar> 2009-10-21 08:07:55 PDT ---
Isn't comparison of structs bitwise by default? I think this is an enhancement, not a bug. If you want that behaviour you should override opEquals in Rectangle.

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

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


David Simcha <dsimcha@yahoo.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |dsimcha@yahoo.com


--- Comment #2 from David Simcha <dsimcha@yahoo.com> 2009-10-21 08:47:16 PDT ---
Not sure if bitwise comparison should or shouldn't be the default, this is subjective.  However, I'd say this could be solved, along with several similar problems, by a function in a new Phobos module called std.mixin:

// Disclaimer:  Quick and dirty, not tested.
enum equalsByOpEquals = q{
    bool opEquals(typeof(this) rhs) {
        foreach(tupleIndex, elem; this.tupleof) {
            if(rhs.tupleof[tupleIndex] != elem) {
                return false;
            }
        }
        return true;
    }
}

Usage:

struct MyStruct {
    mixin(equalsByOpEquals);

    // Other stuff.
}

We could also define a bunch of other stuff like a generic comparison operator for when you only need some arbitrary total ordering for binary searches, etc. and don't particularly care what that ordering is.

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

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



--- Comment #3 from Andrei Alexandrescu <andrei@metalanguage.com> 2009-10-21 08:56:13 PDT ---
(In reply to comment #2)
> Not sure if bitwise comparison should or shouldn't be the default, this is subjective.

Well bitwise comparison breaks encapsulation. If a type defines its own equality operator, it most definitely had its reason. Skipping that is ignoring the encapsulation attempted by the object.

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

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



--- Comment #4 from Steven Schveighoffer <schveiguy@yahoo.com> 2009-10-21 14:15:28 PDT ---
the default opEquals on a struct that contains only an integer member does a bitwise comparison.  You may interpret this as "it does this because this is how you compare integers" or you may interpret this as "it does this because the default opEquals for structs always does bit comparison."  I think according to the spec, the latter is the case, which is consistent with the given behavior.

In that case, I think this should be marked as an enhancement, I don't think it would be a trivial update.  I would mark it as such, but I don't want to step on toes...

Incidentally, it would be a nice enhancement because it would save a lot of boiler-plate code.

(In reply to comment #3)
> Well bitwise comparison breaks encapsulation. If a type defines its own equality operator, it most definitely had its reason. Skipping that is ignoring the encapsulation attempted by the object.

I agree, if you want bitwise comparison, use 'is'.

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

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


Don <clugdbug@yahoo.com.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |clugdbug@yahoo.com.au


--- Comment #5 from Don <clugdbug@yahoo.com.au> 2009-11-13 23:52:28 PST ---
This has some tricky points. The first is that it's recursive. As well as
structs, it also applies to fixed-length arrays: if either contains a struct
with an opEquals, the entire struct must be compared member-by-member; and this
check must be performed recursively.
Secondly, what happens with unions?

struct Rectangle {
   union {
      Point leftBottom;
      int problematic;
   }
}
I think this should probably be an error.
The third difficult part relates to protection. Any of the struct members may
be private and defined in a different module.

If a field in the struct is a class, it probably applies to it as well.

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

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



--- Comment #6 from Andrei Alexandrescu <andrei@metalanguage.com> 2009-11-14 06:10:08 PST ---
(In reply to comment #5)
> This has some tricky points. The first is that it's recursive. As well as structs, it also applies to fixed-length arrays: if either contains a struct with an opEquals, the entire struct must be compared member-by-member; and this check must be performed recursively.

Correct - more precisely, transitive for direct fields. I think the comparison should simply call a template object.structCompare that we define as a library function.

> Secondly, what happens with unions?
> 
> struct Rectangle {
>    union {
>       Point leftBottom;
>       int problematic;
>    }
> }
> I think this should probably be an error.

I think so too.

> The third difficult part relates to protection. Any of the struct members may be private and defined in a different module.
> 
> If a field in the struct is a class, it probably applies to it as well.

I'm not sure how to address this.

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

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



--- Comment #7 from Don <clugdbug@yahoo.com.au> 2009-11-14 12:22:30 PST ---
(In reply to comment #6)
> (In reply to comment #5)
> > This has some tricky points. The first is that it's recursive. As well as structs, it also applies to fixed-length arrays: if either contains a struct with an opEquals, the entire struct must be compared member-by-member; and this check must be performed recursively.
> 
> Correct - more precisely, transitive for direct fields. I think the comparison should simply call a template object.structCompare that we define as a library function.

I'm pretty sure the compiler can just translate it into (x.field1==y.field1 && x.field2==y.field2 && ...) if any field has an opEquals(). This is recursive: so any struct fields which don't have opEquals() can simply use bitwise comparison as now. The recursion doesn't apply to classes; if it doesn't have opEquals(), it's bitwise regardless of what its members do. I guess this is what you mean by direct fields.

> > The third difficult part relates to protection. Any of the struct members may be private and defined in a different module.
> > 
> > If a field in the struct is a class, it probably applies to it as well.
> 
> I'm not sure how to address this.


I have now got this successfully detecting the cases where it needs to use member-by-member comparison for the cases above. I'll add an error for the union case. I don't yet have a solution to the protection issue. Obviously it can done by suppressing any protection errors which occur, but I hope it can be done without a compiler hack.

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

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


Walter Bright <bugzilla@digitalmars.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |bugzilla@digitalmars.com


--- Comment #8 from Walter Bright <bugzilla@digitalmars.com> 2009-11-21 15:16:47 PST ---
I'm working on this. It's a bit of a mess in dmd right now, I'll fix it. I know how to make it work, as it's just like it should in C++. Sigh. The array comparisons are done using TypeInfo's.

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

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


Leandro Lucarella <llucax@gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |llucax@gmail.com


--- Comment #9 from Leandro Lucarella <llucax@gmail.com> 2009-11-22 14:37:32 PST ---
SVN commit: http://www.dsource.org/projects/dmd/changeset/260

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