Revise core.internal.array.equality

kinke · JohanEngelen · commit bfcaa3528048 · 2020-07-30T13:16:17.000+02:00
* Use memcmp for more types (incl. pointers and some static arrays), and don't require both element types to be equivalent (e.g., use it for comparing wchar[] and ushort[] too). * Simplify the non-memcmp branch - the previous code seemed to try to do what the compiler already does. * The previous code for structs without custom opEquals basically enforced -preview=fieldwise via .tupleof comparison. This breaking change with 2.078 almost certainly wasn't intended (and not mentioned in the changelog) and led to undesirable inconsistent behavior, see dlang#3142 (comment). This reverts that change in semantics. * Avoid superfluously nested templates and convert the remaining `at()` helper to a module-level template to reduce the number of redundant instantiations. See dlang#3142
diff --git a/changelog/array_equality.dd b/changelog/array_equality.dd
@@ -0,0 +1,23 @@
+Equality of arrays of structs is consistent again, as before v2.078
+
+Since v2.078, some array equality comparisons (e.g., if both arrays are
+dynamic arrays) have been wrongly using a `.tupleof` comparison for
+structs without custom `opEquals`, basically enforcing
+`-preview=fieldwise` for these array comparisons.
+
+---
+union U
+{
+    string s;
+}
+
+void main()
+{
+    static immutable from = "from", from2 = "from2";
+    U[] a = [{ s : from }];
+    U[1] b = [{ s : from2[0..4] }];
+    assert(a[0] != b[0]);
+    assert(a != b);
+    assert(a != b[]); // worked before v2.078, been failing since, now working again
+}
+---
diff --git a/src/core/internal/array/equality.d b/src/core/internal/array/equality.d
@@ -1,7 +1,7 @@
 /**
- * This module contains compiler support determining equality of dynamic arrays.
+ * This module contains compiler support determining equality of arrays.
  *
- * Copyright: Copyright Digital Mars 2000 - 2019.
+ * Copyright: Copyright Digital Mars 2000 - 2020.
  * License: Distributed under the
  *      $(LINK2 http://www.boost.org/LICENSE_1_0.txt, Boost Software License 1.0).
  *    (See accompanying file LICENSE)
@@ -23,112 +23,47 @@ bool __ArrayEq(T1, T2)(T1[] a, T2[] b)
     return true;
 }
 
- // `lhs == rhs` lowers to `__equals(lhs, rhs)` for dynamic arrays
-bool __equals(T1, T2)(T1[] lhs, T2[] rhs)
+// The compiler lowers `lhs == rhs` to `__equals(lhs, rhs)` for
+// * dynamic arrays,
+// * (most) arrays of different (unqualified) element types, and
+// * arrays of structs with custom opEquals.
+bool __equals(T1, T2)(scope T1[] lhs, scope T2[] rhs)
 {
-    import core.internal.traits : Unqual;
-    alias U1 = Unqual!T1;
-    alias U2 = Unqual!T2;
-
-    static @trusted ref R at(R)(R[] r, size_t i) { return r.ptr[i]; }
-    static @trusted R trustedCast(R, S)(S[] r) { return cast(R) r; }
-
     if (lhs.length != rhs.length)
         return false;
 
-    if (lhs.length == 0 && rhs.length == 0)
+    if (lhs.length == 0)
         return true;
 
-    static if (is(U1 == void) && is(U2 == void))
-    {
-        return __equals(trustedCast!(ubyte[])(lhs), trustedCast!(ubyte[])(rhs));
-    }
-    else static if (is(U1 == void))
-    {
-        return __equals(trustedCast!(ubyte[])(lhs), rhs);
-    }
-    else static if (is(U2 == void))
-    {
-        return __equals(lhs, trustedCast!(ubyte[])(rhs));
-    }
-    else static if (!is(U1 == U2))
+    static if (useMemcmp!(T1, T2))
     {
-        // This should replace src/object.d _ArrayEq which
-        // compares arrays of different types such as long & int,
-        // char & wchar.
-        // Compiler lowers to __ArrayEq in dmd/src/opover.d
-        foreach (const u; 0 .. lhs.length)
-        {
-            if (at(lhs, u) != at(rhs, u))
-                return false;
-        }
-        return true;
-    }
-    else static if (__traits(isIntegral, U1))
-    {
-
         if (!__ctfe)
         {
-            import core.stdc.string : memcmp;
-            return () @trusted { return memcmp(cast(void*)lhs.ptr, cast(void*)rhs.ptr, lhs.length * U1.sizeof) == 0; }();
+            static bool trustedMemcmp(scope T1[] lhs, scope T2[] rhs) @trusted @nogc nothrow pure
+            {
+                pragma(inline, true);
+                import core.stdc.string : memcmp;
+                return memcmp(cast(void*) lhs.ptr, cast(void*) rhs.ptr, lhs.length * T1.sizeof) == 0;
+            }
+            return trustedMemcmp(lhs, rhs);
         }
         else
         {
-            foreach (const u; 0 .. lhs.length)
+            foreach (const i; 0 .. lhs.length)
             {
-                if (at(lhs, u) != at(rhs, u))
+                if (at(lhs, i) != at(rhs, i))
                     return false;
             }
             return true;
         }
     }
     else
     {
-        foreach (const u; 0 .. lhs.length)
+        foreach (const i; 0 .. lhs.length)
         {
-            static if (__traits(compiles, __equals(at(lhs, u), at(rhs, u))))
-            {
-                if (!__equals(at(lhs, u), at(rhs, u)))
-                    return false;
-            }
-            else static if (__traits(isFloating, U1))
-            {
-                if (at(lhs, u) != at(rhs, u))
-                    return false;
-            }
-            else static if (is(U1 : Object) && is(U2 : Object))
-            {
-                if (!(cast(Object)at(lhs, u) is cast(Object)at(rhs, u)
-                    || at(lhs, u) && (cast(Object)at(lhs, u)).opEquals(cast(Object)at(rhs, u))))
-                    return false;
-            }
-            else static if (__traits(hasMember, U1, "opEquals"))
-            {
-                if (!at(lhs, u).opEquals(at(rhs, u)))
-                    return false;
-            }
-            else static if (is(U1 == delegate))
-            {
-                if (at(lhs, u) != at(rhs, u))
-                    return false;
-            }
-            else static if (is(U1 == U11*, U11))
-            {
-                if (at(lhs, u) != at(rhs, u))
-                    return false;
-            }
-            else static if (__traits(isAssociativeArray, U1))
-            {
-                if (at(lhs, u) != at(rhs, u))
-                    return false;
-            }
-            else
-            {
-                if (at(lhs, u).tupleof != at(rhs, u).tupleof)
-                    return false;
-            }
+            if (at(lhs, i) != at(rhs, i))
+                return false;
         }
-
         return true;
     }
 }
@@ -203,3 +138,79 @@ bool __equals(T1, T2)(T1[] lhs, T2[] rhs)
     assert(!__equals(a1, a2));
     assert(a1 != a2);
 }
+
+
+private:
+
+// - Recursively folds static array types to their element type,
+// - maps void to ubyte, and
+// - pointers to size_t.
+template BaseType(T)
+{
+    static if (__traits(isStaticArray, T))
+        alias BaseType = BaseType!(typeof(T.init[0]));
+    else static if (is(immutable T == immutable void))
+        alias BaseType = ubyte;
+    else static if (is(T == E*, E))
+        alias BaseType = size_t;
+    else
+        alias BaseType = T;
+}
+
+// Use memcmp if the element sizes match and both base element types are integral.
+// Due to int promotion, disallow small integers of diverging signed-ness though.
+template useMemcmp(T1, T2)
+{
+    static if (T1.sizeof != T2.sizeof)
+        enum useMemcmp = false;
+    else
+    {
+        alias B1 = BaseType!T1;
+        alias B2 = BaseType!T2;
+        enum useMemcmp = __traits(isIntegral, B1) && __traits(isIntegral, B2)
+           && !( (B1.sizeof < 4 || B2.sizeof < 4) && __traits(isUnsigned, B1) != __traits(isUnsigned, B2) );
+    }
+}
+
+unittest
+{
+    enum E { foo, bar }
+
+    static assert(useMemcmp!(byte, byte));
+    static assert(useMemcmp!(ubyte, ubyte));
+    static assert(useMemcmp!(void, const void));
+    static assert(useMemcmp!(void, immutable bool));
+    static assert(useMemcmp!(void, inout char));
+    static assert(useMemcmp!(void, shared ubyte));
+    static assert(!useMemcmp!(void, byte));       // differing signed-ness
+    static assert(!useMemcmp!(char[8], byte[8])); // ditto
+
+    static assert(useMemcmp!(short, short));
+    static assert(useMemcmp!(wchar, ushort));
+    static assert(!useMemcmp!(wchar, short)); // differing signed-ness
+
+    static assert(useMemcmp!(int, uint)); // no promotion, ignoring signed-ness
+    static assert(useMemcmp!(dchar, E));
+
+    static assert(useMemcmp!(immutable void*, size_t));
+    static assert(useMemcmp!(double*, ptrdiff_t));
+    static assert(useMemcmp!(long[2][3], const(ulong)[2][3]));
+
+    static assert(!useMemcmp!(float, float));
+    static assert(!useMemcmp!(double[2], double[2]));
+    static assert(!useMemcmp!(Object, Object));
+    static assert(!useMemcmp!(int[], int[]));
+}
+
+// Returns a reference to an array element, eliding bounds check and
+// casting void to ubyte.
+pragma(inline, true)
+ref at(T)(T[] r, size_t i) @trusted
+    // exclude opaque structs due to https://issues.dlang.org/show_bug.cgi?id=20959
+    if (!(is(T == struct) && !is(typeof(T.sizeof))))
+{
+    static if (is(immutable T == immutable void))
+        return (cast(ubyte*) r.ptr)[i];
+    else
+        return r.ptr[i];
+}
