use core.internal.hash for TypeInfo.getHash #2243

IgorStepanov · 2018-07-05T20:37:51Z

Use the new core.internal.hash.hashOf in all TypeInfo.getHash instead of the old rt.util.hash.hashOf.
Also make typeid(T).getHash(&val) to get the same result with hashOf(val).

dlang-bot · 2018-07-05T20:37:52Z

Thanks for your pull request and interest in making D better, @IgorStepanov! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + druntime#2243"

IgorStepanov · 2018-07-05T23:19:24Z

Please, note, tests in another projects, which fails on continuous-integration/jenkins/pr-merge depends on associative array order:
for example, in ae xml build expects result string
<svg xmlns="http://www.w3.org/2000/svg" version="1.1"><text fill="red" x="0" y="15">I love SVG</text></svg>
or
<svg xmlns="http://www.w3.org/2000/svg" version="1.1"><text x="0" y="15" fill="red">I love SVG</text></svg>

After this PR, result string is
<svg xmlns="http://www.w3.org/2000/svg" version="1.1"><text fill="red" y="15" x="0">I love SVG</text></svg>
Obviously this fails is a test issue and doesn't threaten real user code.

wilzbach · 2018-07-05T23:29:44Z

Please, note, tests in another projects, which fails on continuous-integration/jenkins/pr-merge

FYI before this PR can be merged, the Project Tester needs to be green.
In other words: ae needs to pass on the Project Tester.
CC @CyberShadow

Obviously this fails is a test issue and doesn't threaten real user code.

Yes, but we still want our CIs to be green ;-)

CyberShadow · 2018-07-06T11:44:21Z

Yeah, that's a bad test. Fixed & pushed tag.

n8sh · 2018-07-09T10:41:54Z

src/core/internal/hash.d

    //class or interface array or struct array with toHash(); CTFE depend on toHash() method
    //also do this for arrays of structs whose hashes would be calculated in a memberwise fashion
    {
        size_t hash = seed;
        foreach (o; val)
        {
-            hash = hashOf(o, hash);
+            hash = hashOf(hashOf(o), hash); //double hashing because TypeInfo.getHash doesn't allow to pass seed value


~~Please explain why this is necessary. Does this patch make hashOf(o, hash) not compile?~~

Ah nevermind, I see

Also make typeid(T).getHash(&val) to get the same result with hashOf(val).

n8sh · 2018-07-09T10:45:10Z

src/rt/util/container/hashtab.d

        else
-            return hashOf((&key)[0 .. 1], 0);
+            return .hashOf((&key)[0 .. 1], 0);


This changes the meaning! Is that intentional? The old version was hashing the key as bytes.

I use object.hashOf instead of rt.util.hash : hashOf, because rt.util.hash has been removed.
I may force use object.hashOf(void[]) if we strongly want bytes hash, but I think it is not needed.

n8sh · 2018-07-09T20:15:38Z

src/rt/aaA.d

@@ -672,25 +672,28 @@ extern (C) int _aaEqual(in TypeInfo tiRaw, in AA aa1, in AA aa2)
 extern (C) hash_t _aaGetHash(in AA* aa, in TypeInfo tiRaw) nothrow
 {
    if (aa.empty)
-        return 0;
+        return hashOf(0);


Suggestion: revert this to return 0 and change core.internal.hash to match.

core.internal.hash.hashOf may use seed value.
The current function looks like

size_t hashOf(T)(T aa, size_t seed = 0) if (!is(T == enum) && __traits(isAssociativeArray, T)) { if (!aa.length) return hashOf(0, seed); }

If I rewrite it as...

size_t hashOf(T)(T aa, size_t seed = 0) if (!is(T == enum) && __traits(isAssociativeArray, T)) { if (!aa.length) return seed; }

... we will disable way to chain several hashes:

a = hashOf(cast(int[int])null, hashOf(cast(int[int])null)); b = hashOf(cast(int[int])null, hashOf(cast(int[int])null, hashOf(cast(int[int])null))); // a == b but a should differs b

as result:
hashes of two arrays with different count of null AA will be same:

auto arr = [int[int].init, int[int].init]; auto arr2 = [int[int].init, int[int].init, int[int].init]; assert(hashOf(arr) != hashOf(arr2)); // will be failed

n8sh · 2018-07-09T20:18:29Z

src/rt/typeinfo/ti_cent.d

@@ -30,7 +28,7 @@ class TypeInfo_zi : TypeInfo

    override size_t getHash(scope const void* p)
    {
-        return rt.util.hash.hashOf(p[0 .. cent.sizeof], 0);
+        return hashOf(p[0 .. cent.sizeof], 0);


To match what's in core.internal.hash you should instead write:

return hashOf(*cast(const ucent*) p);

n8sh · 2018-07-09T20:20:33Z

src/rt/typeinfo/ti_delegate.d

@@ -28,7 +27,7 @@ class TypeInfo_D : TypeInfo

    override size_t getHash(scope const void* p)
    {
-        return rt.util.hash.hashOf(p[0 .. dg.sizeof], 0);
+        return hashOf(*cast(dg*)p, 0);


No need for 0 second argument. Currently it will be inferred, and in the future an explicit chaining argument might result in different behavior (#2246).

n8sh · 2018-07-09T20:21:00Z

src/rt/typeinfo/ti_long.d

@@ -28,7 +26,7 @@ class TypeInfo_l : TypeInfo

    override size_t getHash(scope const void* p)
    {
-        return rt.util.hash.hashOf(p[0 .. long.sizeof], 0);
+        return hashOf(*cast(long*)p, 0);


No need for 0 second argument. Currently it will be inferred, and in the future an explicit chaining argument might result in different behavior (#2246).

n8sh · 2018-07-09T20:21:52Z

src/rt/typeinfo/ti_ucent.d

@@ -30,7 +28,7 @@ class TypeInfo_zk : TypeInfo

    override size_t getHash(scope const void* p)
    {
-        return rt.util.hash.hashOf(p[0 .. ucent.sizeof], 0);
+        return hashOf(p[0 .. ucent.sizeof], 0);


To match what's in core.internal.hash you should instead write:

return hashOf(*cast(const ucent*) p);

n8sh · 2018-07-09T22:30:49Z

src/object.d

@@ -1703,7 +1703,7 @@ class TypeInfo_Class : TypeInfo
    override size_t getHash(scope const void* p) @trusted const
    {
        auto o = *cast(Object*)p;
-        return o ? o.toHash() : 0;
+        return hashOf(o ? o.toHash() : 0);


If #2246 is accepted you will be able to revert this to o ? o.toHash() : 0.

n8sh · 2018-07-09T22:31:38Z

src/object.d

@@ -1930,14 +1934,11 @@ class TypeInfo_Struct : TypeInfo
        assert(p);
        if (xtoHash)
        {
-            return (*xtoHash)(p);
+            return hashOf((*xtoHash)(p));


If #2246 is accepted you will be able to revert this to return (*xtoHash)(p);

n8sh · 2018-07-09T22:32:11Z

src/object.d

        Interface* pi = **cast(Interface ***)*cast(void**)p;
        Object o = cast(Object)(*cast(void**)p - pi.offset);
        assert(o);
-        return o.toHash();
+        return hashOf(o.toHash());


If #2246 is accepted you will be able to revert this to return o.toHash();

n8sh · 2018-07-09T22:38:18Z

src/rt/typeinfo/ti_n.d

@@ -21,7 +21,7 @@ class TypeInfo_n : TypeInfo

    override size_t getHash(scope const void* p) const
    {
-        return 0;
+        return hashOf(cast(void*)null);


Post #2246 this should be the same as return 0.

Same above:
hashOf([null, null]) and hashOf([null, null, null]) should have different values, thus we need chaining by seed, thus we need to rehash seed 0 each time.

n8sh · 2018-07-09T22:40:00Z

src/rt/typeinfo/ti_ulong.d

@@ -28,7 +27,7 @@ class TypeInfo_m : TypeInfo

    override size_t getHash(scope const void* p)
    {
-        return rt.util.hash.hashOf(p[0 .. ulong.sizeof], 0);
+        return hashOf(*cast(ulong*)p, 0);


No need for 0 second argument. Currently it will be inferred, and in the future an explicit chaining argument might result in different behavior (#2246).

IgorStepanov · 2018-07-23T16:20:51Z

@wilzbach ping

wilzbach · 2018-07-23T19:14:22Z

I think we should add a small changelog entry stating that the default toHash method has been changed to use hashOf and thus as an resulted e.g. tests which fasly assume that the order of built-in associative arrays is fixed, may fail.

Would be nice to still have this as this should raise awareness in case someone runs into similar issues. It's simple about adding a file to the changelog folder.
See https://github.com/dlang/druntime/tree/master/changelog

IgorStepanov · 2018-07-28T14:20:07Z

@wilzbach Thanks!

Would be nice to still have this as this should raise awareness in case someone runs into similar issues. It's simple about adding a file to the changelog folder.

Something like #2256 ?

add changelog for PR #2243 merged-on-behalf-of: Jacob Carlborg <jacob-carlborg@users.noreply.github.com>

dlang/druntime#2243 was intended to make `typeid(x).getHash(&x)` match `hashOf(x)` but did not account for structs with auto-generated `__xtoHash`.

IgorStepanov requested review from andralex, dnadlinger, MartinNowak and schveiguy as code owners July 5, 2018 20:37

IgorStepanov requested review from WalterBright and wilzbach as code owners July 5, 2018 20:37

IgorStepanov mentioned this pull request Jul 5, 2018

[trivial] Fix AA tests: order of AA elements isn't determined dlang/phobos#6628

Merged

IgorStepanov force-pushed the typeinfo-gethash-tohash branch from e592ba2 to 1821686 Compare July 5, 2018 23:04

IgorStepanov force-pushed the typeinfo-gethash-tohash branch 3 times, most recently from 05917ea to 7aac1c1 Compare July 6, 2018 22:12

IgorStepanov mentioned this pull request Jul 6, 2018

fix Bson.opEquals: order of object keys is not determined vibe-d/vibe.d#2183

Merged

n8sh reviewed Jul 9, 2018

View reviewed changes

n8sh mentioned this pull request Jul 9, 2018

Fix Issue 19071 - core.internal.hash should have non-chained toHash overloads #2246

Merged

n8sh reviewed Jul 9, 2018

View reviewed changes

IgorStepanov mentioned this pull request Jul 10, 2018

fix tests: order of AA elems is not determined BlackEdder/ggplotd#51

Merged

wilzbach added the 72h no objection -> merge The PR will be merged if there are no objections raised. label Jul 20, 2018

use core.internal.hash for TypeInfo.getHash

939b8aa

IgorStepanov force-pushed the typeinfo-gethash-tohash branch from a5b15c1 to 939b8aa Compare July 22, 2018 23:38

wilzbach merged commit cb5efa9 into dlang:master Jul 23, 2018

n8sh mentioned this pull request Jul 27, 2018

Fix Issue 19073 - core.internal.hash should not bitwise hash representations of floating point numbers, plus minor improvements and refactoring #2240

Merged

IgorStepanov added a commit to IgorStepanov/druntime that referenced this pull request Jul 28, 2018

add changelog for PR dlang#2243

7b5223e

IgorStepanov added a commit to IgorStepanov/druntime that referenced this pull request Jul 28, 2018

add changelog for PR dlang#2243

dc3c763

IgorStepanov added a commit to IgorStepanov/druntime that referenced this pull request Jul 28, 2018

add changelog for PR dlang#2243

29fa9aa

IgorStepanov mentioned this pull request Jul 28, 2018

add changelog for PR #2243 #2256

Merged

IgorStepanov added a commit to IgorStepanov/druntime that referenced this pull request Jul 28, 2018

add changelog for PR dlang#2243

fc3b6ed

IgorStepanov added a commit to IgorStepanov/druntime that referenced this pull request Jul 28, 2018

add changelog for PR dlang#2243

00e2ec5

dlang-bot added a commit that referenced this pull request Jul 28, 2018

Merge pull request #2256 from IgorStepanov/pr-2243-changelog

52d3fe0

add changelog for PR #2243 merged-on-behalf-of: Jacob Carlborg <jacob-carlborg@users.noreply.github.com>

n8sh mentioned this pull request Sep 8, 2018

core.internal.hash prefer FNV to MurmurHash3 for short sequences of statically known length #2295

Merged

n8sh mentioned this pull request Sep 8, 2018

[DO NOT MERGE] Have __xtoHash delegate to core.internal.hash.hashOf so the two match dlang/dmd#8676

Open

n8sh mentioned this pull request Aug 1, 2020

Templated TypeInfo #3174

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use core.internal.hash for TypeInfo.getHash #2243

use core.internal.hash for TypeInfo.getHash #2243

IgorStepanov commented Jul 5, 2018

dlang-bot commented Jul 5, 2018

IgorStepanov commented Jul 5, 2018

wilzbach commented Jul 5, 2018

CyberShadow commented Jul 6, 2018

n8sh Jul 9, 2018 •

edited

n8sh Jul 9, 2018

n8sh Jul 9, 2018

IgorStepanov Jul 10, 2018

n8sh Jul 9, 2018

IgorStepanov Jul 10, 2018

n8sh Jul 9, 2018

IgorStepanov Jul 10, 2018

n8sh Jul 9, 2018

IgorStepanov Jul 10, 2018

n8sh Jul 9, 2018

IgorStepanov Jul 10, 2018

n8sh Jul 9, 2018

IgorStepanov Jul 10, 2018

n8sh Jul 9, 2018

n8sh Jul 9, 2018

n8sh Jul 9, 2018

n8sh Jul 9, 2018

IgorStepanov Jul 10, 2018

n8sh Jul 9, 2018

IgorStepanov Jul 10, 2018

IgorStepanov commented Jul 23, 2018

wilzbach commented Jul 23, 2018

IgorStepanov commented Jul 28, 2018

use core.internal.hash for TypeInfo.getHash #2243

use core.internal.hash for TypeInfo.getHash #2243

Conversation

IgorStepanov commented Jul 5, 2018

dlang-bot commented Jul 5, 2018

Bugzilla references

Testing this PR locally

IgorStepanov commented Jul 5, 2018

wilzbach commented Jul 5, 2018

CyberShadow commented Jul 6, 2018

n8sh Jul 9, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IgorStepanov commented Jul 23, 2018

wilzbach commented Jul 23, 2018

IgorStepanov commented Jul 28, 2018

n8sh Jul 9, 2018 •

edited