Review request: getHash candidate for TypeInfo_AssociativeArray #171

Merged
merged 4 commits into from Nov 18, 2012

3 participants

@quickfur
D Programming Language member

To address AA problems such as issue 6210 and issue 7371, I wrote an override for getHash in TypeInfo_AssociativeArray. I've tested the code and confirmed it fixes issues 6210 and 7371. Basically it iterates over key/value pairs, and computes the hash of the key/value's respective hashes, summing the hashes over all pairs. (See code comments for explanation.)

However, I'm not sure if aaA.d is the best place to put this code. It adds another function to the aaA.d API, which changes D's AA ABI. However, I'm not sure if this function is implementable in object_.d without really dirty hacks such as struct AssociativeArray(K,V).{Slot,Hashtable} (and even with those, I don't know how to instantiate AssociativeArray given only the associated TypeInfo object).

Comments?

@quickfur
D Programming Language member

Rebased to prevent stagnation.

@andralex andralex commented on an outdated diff Jul 8, 2012
src/rt/aaA.d
@@ -796,6 +796,32 @@ BB* _d_assocarrayliteralTX(TypeInfo_AssociativeArray ti, void[] keys, void[] val
}
+static TypeInfo_AssociativeArray _aaUnwrapTypeInfo(TypeInfo tiRaw)
+{
+ TypeInfo_AssociativeArray ti;
+ while (true)
+ {
+ if ((ti = cast(TypeInfo_AssociativeArray)tiRaw) !is null)
+ break;
+ else if (auto tiConst = cast(TypeInfo_Const)tiRaw) {
@andralex
D Programming Language member
andralex added a line comment Jul 8, 2012

remove "else"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@andralex andralex and 1 other commented on an outdated diff Jul 8, 2012
src/rt/aaA.d
+ {
+ if ((ti = cast(TypeInfo_AssociativeArray)tiRaw) !is null)
+ break;
+ else if (auto tiConst = cast(TypeInfo_Const)tiRaw) {
+ // The member in object_.d and object.di differ. This is to ensure
+ // the file can be compiled both independently in unittest and
+ // collectively in generating the library. Fixing object.di
+ // requires changes to std.format in Phobos, fixing object_.d
+ // makes Phobos's unittest fail, so this hack is employed here to
+ // avoid irrelevant changes.
+ static if (is(typeof(&tiConst.base) == TypeInfo*))
+ tiRaw = tiConst.base;
+ else
+ tiRaw = tiConst.next;
+ } else
+ assert(0); // ???
@andralex
D Programming Language member
andralex added a line comment Jul 8, 2012

Add a note mentioning the bugs if this is ever reached.

@quickfur
D Programming Language member
quickfur added a line comment Jul 16, 2012

Hi Andrei, this code was moved verbatim from __aaEqual so that it can be reused. I don't know the reason for the original assert.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@andralex andralex commented on an outdated diff Jul 8, 2012
src/rt/aaA.d
@@ -903,3 +911,78 @@ int _aaEqual(TypeInfo tiRaw, AA e1, AA e2)
return 1; // equal
}
+
+
+/*****************************************
+ * Computes a hash value for the entire AA
+ * Returns:
+ * Hash value
+ */
+extern (C)
+hash_t _aaGetHash(AA* aa, TypeInfo tiRaw)
+{
+ import rt.util.hash;
+
+ hash_t h = 0;
+
+ if (aa.a)
@andralex
D Programming Language member
andralex added a line comment Jul 8, 2012

This guards a large portion of the code. Better yet mention

if (!aa.a) 
{
    return 0;
}

at the very beginning of the function, after which you get to handle the real case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@andralex andralex commented on an outdated diff Jul 8, 2012
src/rt/aaA.d
+
+ foreach (e; aa.a.b)
+ {
+ while (e)
+ {
+ auto pkey = cast(void*)(e + 1);
+ auto pvalue = pkey + keysize;
+
+ // Compute a hash for the key/value pair by hashing their
+ // respective hash values.
+ hash_t[2] hpair;
+ hpair[0] = e.hash;
+ hpair[1] = valueti.getHash(pvalue);
+
+ // Combine the hash of the key/value pair with the running hash
+ // value using a commutative operator (+) so that the resulting
@andralex
D Programming Language member
andralex added a line comment Jul 8, 2012

s/commutative/associative/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@andralex
D Programming Language member

@quickfur What's the status of this? Thanks!

@andralex
D Programming Language member

ping?

@quickfur
D Programming Language member

Rebased & updated per Andrei's comments.

@andralex
D Programming Language member

This now breaks the builds, presumably because of the const changes (some of which I'll undo). Stay tuned and please fix for next week. Thanks!

@quickfur
D Programming Language member

Are you reverting making toHash const? The changes with const toHash are non-trivial. If you're reverting making toHash const, I'm going to wait until afterwards.

@andralex
D Programming Language member

Honest I'm not sure what the best path is for toHash. @9rnsr ?

@andralex
D Programming Language member

ping @9rnsr and @quickfur. How should we move forward with this?

@andralex
D Programming Language member

reping

@quickfur
D Programming Language member

Sorry for the late update; this morning I finally figured out a simple way to fix the code to work with const toHash without requiring massive changes. So this has been rebased and is ready to merge now.

@quickfur
D Programming Language member

Rebased to avoid stagnation

@quickfur
D Programming Language member

Rebased to avoid stagnation.

@andralex andralex was assigned Oct 26, 2012
@alexrp
D Programming Language member

Assigning to @andralex

@quickfur
D Programming Language member

Should I squash it too?

@alexrp
D Programming Language member

@andralex waiting on you

@andralex
D Programming Language member

worksforme

@andralex andralex merged commit 0650b6e into dlang:master Nov 18, 2012

1 check was pending

Details default Pass: 6, Pending: 3
@andralex
D Programming Language member

merged, thanks!

@quickfur quickfur deleted the quickfur:gethash_fixes branch Jul 16, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment