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


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


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


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


--- 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: -------
Nov 13 2009
prev sibling next sibling parent d-bugmail puremagic.com writes:
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: -------
Nov 14 2009
prev sibling next sibling parent d-bugmail puremagic.com writes:
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: -------
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


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


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


--- Comment #10 from Walter Bright <bugzilla digitalmars.com> 2009-12-06
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