_equals druntime template for structs#1792
Conversation
src/object.d
Outdated
| bool __equals(T1, T2)(T1[] lhs, T2[] rhs) | ||
| { | ||
| if (lhs.length != rhs.length) | ||
| return 0; |
There was a problem hiding this comment.
stylistic nitpick: if you're going to use return true; at the end, then make this return false;
| enum RTInfo = null; | ||
| } | ||
|
|
||
| bool __equals(T1, T2)(T1[] lhs, T2[] rhs) |
There was a problem hiding this comment.
Please add a comment before this function a la // lhs == rhs lowers to __equals(lhs, rhs) for arrays of all struct types.
src/object.d
Outdated
| import core.internal.traits : Unqual; | ||
| alias U1 = Unqual!T1; | ||
| alias U2 = Unqual!T2; | ||
| static assert(is(U1 == U2), "Internal error."); |
There was a problem hiding this comment.
No need to define names because we're never using them, static assert(is(Unqual!T1 == Unqual!T2), "Internal error."); should suffice
src/object.d
Outdated
| foreach (const u; 0 .. lhs.length) | ||
| { | ||
| auto s1 = at(lhs, u); | ||
| auto s2 = at(rhs, u); |
There was a problem hiding this comment.
Don't create temporaries here, just invoke at inline; the structs may not be copyable
src/object.d
Outdated
|
|
||
| static if (__traits(compiles, s1.opEquals(s2))) | ||
| { | ||
| auto result = (s1).opEquals(s2); |
src/object.d
Outdated
| } | ||
| else | ||
| { | ||
| return s1.tupleof == s2.tupleof; |
There was a problem hiding this comment.
if (s1.tupleof != s2.tupleof)
return false;There was a problem hiding this comment.
You should write a unittest that fails with the current code and passes with the correct one.
There was a problem hiding this comment.
I'm not sure I understand what you mean here.
There was a problem hiding this comment.
Nice. I had a case where both assert(a != b); and assert(a==b); passed with that code because I forgot to do this in the compiler:
if (op == TOKnotequal)
{
__equals = new NotExp(loc, __equals);
}
src/object.d
Outdated
| { | ||
| auto result = (s1).opEquals(s2); | ||
| if (!result) | ||
| return result; |
There was a problem hiding this comment.
A bit much pomp and circumstance. This should be enough:
if (!s1.opEquals(s2))
return false;
src/object.d
Outdated
| } | ||
| else | ||
| { | ||
| if(!(at(lhs, u).tupleof == at(rhs, u).tupleof)) |
|
|
||
| assert(arr1 != arr2); | ||
| assert(arr2 == arr3); | ||
| } |
There was a problem hiding this comment.
Need to also add a unittest for a struct that defines opEquals for coverage.
4 participants
This is similar to what happened to
adCmp, or__cmp, only that it's a first step, rather than the whole implementation. This is the template function for testing equality on arrays of structs (basetype is struct).I also worked on a
dmdpatch loweringEqualExpto this code for arrays of structs. Locally,DruntimeandPhobostests are passing with the new implementation.__equalsis a bit more interesting than__cmpbecause for arrays of basic types the compiler makes an optimization and generates a call too "memcmp" directly rather than call adruntimefunction. See here: https://github.com/dlang/dmd/blob/master/src/ddmd/e2ir.d#L2282I avoided changing that bit for the time being until we can agree on what the best approach is. This is one of the reasons this PR only does struct array comparisons. The other would be that a smaller PR is easier to review.