www.digitalmars.com         C & C++   DMDScript  

digitalmars.D.bugs - [Issue 3433] New: [tdpl] Comparing structs for equality is not member-by-member

reply d-bugmail puremagic.com writes:
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



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: -------
Oct 21 2009
next sibling parent d-bugmail puremagic.com writes:
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



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: -------
Oct 21 2009
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=3433


David Simcha <dsimcha yahoo.com> changed:

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



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: -------
Oct 21 2009
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=3433




08:56:13 PDT ---


 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: -------
Oct 21 2009
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=3433




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.



 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: -------
Oct 21 2009
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=3433


Don <clugdbug yahoo.com.au> changed:

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



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: -------
Nov 13 2009
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=3433




06:10:08 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.


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: -------
Nov 14 2009
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=3433








 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: -------
Nov 14 2009
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=3433


Walter Bright <bugzilla digitalmars.com> changed:

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



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: -------
Nov 21 2009
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=3433


Leandro Lucarella <llucax gmail.com> changed:

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



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: -------
Nov 22 2009
prev sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=3433


Walter Bright <bugzilla digitalmars.com> changed:

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



00:53:45 PST ---
Fixed dmd 2.037

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