Bug 3433 – [tdpl] Comparing structs for equality is not member-by-member

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.

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.

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.

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

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

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.

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

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

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.

SVN commit: http://www.dsource.org/projects/dmd/changeset/260

Fixed dmd 2.037