From cdc23f3a61c29c0c283814816cd05b30a82d0b8e Mon Sep 17 00:00:00 2001 From: Martin Kinkelin Date: Wed, 3 Jun 2020 01:34:39 +0200 Subject: [PATCH] Revise core.internal.array.equality * 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 https://github.com/dlang/druntime/pull/3142#issuecomment-648400109. 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. --- changelog/array_equality.dd | 23 ++++ src/core/internal/array/equality.d | 179 ++++++++++++++++------------- 2 files changed, 120 insertions(+), 82 deletions(-) create mode 100644 changelog/array_equality.dd diff --git a/changelog/array_equality.dd b/changelog/array_equality.dd new file mode 100644 index 0000000000..17485ea2e7 --- /dev/null +++ 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 index 6c0a3092ee..1dd7de1e6a 100644 --- 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) @@ -10,56 +10,35 @@ module core.internal.array.equality; - // `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)) { - 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; @@ -67,51 +46,11 @@ bool __equals(T1, T2)(T1[] lhs, T2[] rhs) } 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; } } @@ -186,3 +125,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]; +}