Skip to content
This repository was archived by the owner on Oct 12, 2022. It is now read-only.
/ druntime Public archive

use array interface to hashOf() #1595

Merged
andralex merged 1 commit into from
Jun 25, 2016
Merged

use array interface to hashOf() #1595

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 17 additions & 9 deletions src/object.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,11 +214,11 @@ class TypeInfo
{
import core.internal.traits : externDFunc;
alias hashOf = externDFunc!("rt.util.hash.hashOf",
size_t function(const(void)*, size_t, size_t) @trusted pure nothrow);
size_t function(const(void)[], size_t) @trusted pure nothrow @nogc);
try
{
auto data = this.toString();
return hashOf(data.ptr, data.length, 0);
return hashOf(data, 0);
}
catch (Throwable)
{
Expand Down Expand Up @@ -255,7 +255,15 @@ class TypeInfo
return ti && this.toString() == ti.toString();
}

/// Returns a hash of the instance of a type.
/**
* Computes a hash of the instance of a type.
* Params:
* p = pointer to start of instance of the type
* Returns:
* the hash
* Bugs:
* fix https://issues.dlang.org/show_bug.cgi?id=12516 e.g. by changing this to a truly safe interface.
*/
size_t getHash(in void* p) @trusted nothrow const { return cast(size_t)p; }

/// Compares two instances for equality.
Expand Down Expand Up @@ -1101,7 +1109,7 @@ class TypeInfo_Struct : TypeInfo
this.initializer().length == s.initializer().length;
}

override size_t getHash(in void* p) @safe pure nothrow const
override size_t getHash(in void* p) @trusted pure nothrow const
Copy link
Member Author

@WalterBright WalterBright Jun 17, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the slicing is an unsafe operation. The older code was unsafe as well.

Copy link
Contributor

@dnadlinger dnadlinger Jun 17, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can this interface (dereferencing a void*) even be @safe in the first place?

Copy link
Member Author

@WalterBright WalterBright Jun 17, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getHash is supposed to take a pointer to the object matching the TypeInfo.

Copy link
Contributor

@dnadlinger dnadlinger Jun 17, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"is supposed" is something entirely different than "is guaranteed to". You could of course argue that the interface was broken before, but it still remains a safety violation that should at least be recorded in Bugzilla (and subsequently be deprecated/made un-@safe).

Copy link
Member Author

@WalterBright WalterBright Jun 17, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do add a bugzilla issue for it. Such is beyond the scope of this PR, however, and should not impede it.

Copy link
Contributor

@dnadlinger dnadlinger Jun 17, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do add a bugzilla issue for it. Such is beyond the scope of this PR, however, and should not impede it.

The PR adds a @trusted annotation that is clearly wrong. It's barely acceptable because the code was already broken before, but now it is your responsibility to make sure it is eventually cleaned up. Previously, the place would have been flagged automatically once the mistaken @trusted annotation on hashOf is removed.

Copy link
Member Author

@WalterBright WalterBright Jun 17, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The @safe was equally wrong. This PR does not address (or change) the interface to getHash(), the change was made to reflect the unsafe operation inside the function. The issue you brought up is with the interface to getHash(), not the interface to hashOf().

your responsibility

It is the responsibility of people who find bugs to post them to bugzilla. The bug you found has nothing to do with this PR, which is to fix the interface to hashOf(). You should take the credit for finding the issue by posting it to bugzilla.

Meanwhile, this PR is only concerned with the interface to hashOf.

Copy link
Contributor

@dnadlinger dnadlinger Jun 17, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The @safe was equally wrong.

No; @safe can never be wrong as in breaking the type system, it can just cause code not to compile. You are adding a new, incorrect instance of @trusted, which does add an extra type system violation.

As I'm sure you realise, this is a huge deal because instances where things are mistakenly marked as @safe can be detected automatically, which is impossible for mistaken uses of @trusted. The only way to find those is to painstakingly go through all occurrences of @trusted and manually audit each one of them.

Just trust me on that one. I've found a large share of the Phobos safety bugs you went through recently, this PR would add another such bug, I point it out, you fix/document it before it goes in. That's how pull request review works. You can't justify knowingly introducing an issue without any documentation, etc. by pointing out that the code base contained bugs before.

{
assert(p);
if (xtoHash)
Expand All @@ -1112,8 +1120,8 @@ class TypeInfo_Struct : TypeInfo
{
import core.internal.traits : externDFunc;
alias hashOf = externDFunc!("rt.util.hash.hashOf",
size_t function(const(void)*, size_t, size_t) @trusted pure nothrow);
return hashOf(p, initializer().length, 0);
size_t function(const(void)[], size_t) @trusted pure nothrow @nogc);
return hashOf(p[0 .. initializer().length], 0);
}
}

Expand Down Expand Up @@ -3167,7 +3175,7 @@ struct Test
size_t hashOf(T)(auto ref T arg, size_t seed = 0)
{
import core.internal.hash;
return core.internal.hash.hashOf(arg, seed);
return core.internal.hash.hashOf((cast(void*)&arg)[0 .. T.sizeof], seed);
}

bool _xopEquals(in void*, in void*)
Expand Down Expand Up @@ -3238,9 +3246,9 @@ private size_t getArrayHash(in TypeInfo element, in void* ptr, in size_t count)

import core.internal.traits : externDFunc;
alias hashOf = externDFunc!("rt.util.hash.hashOf",
size_t function(const(void)*, size_t, size_t) @trusted pure nothrow);
size_t function(const(void)[], size_t) @trusted pure nothrow @nogc);
if(!hasCustomToHash(element))
return hashOf(ptr, elementSize * count, 0);
return hashOf(ptr[0 .. elementSize * count], 0);

size_t hash = 0;
foreach(size_t i; 0 .. count)
Expand Down
4 changes: 2 additions & 2 deletions src/rt/typeinfo/ti_Ag.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@ class TypeInfo_Ag : TypeInfo_Array

override size_t getHash(in void* p) @trusted const
{
byte[] s = *cast(byte[]*)p;
return rt.util.hash.hashOf(s.ptr, s.length * byte.sizeof);
const s = *cast(const void[]*)p;
return rt.util.hash.hashOf(s, 0);
}

override bool equals(in void* p1, in void* p2) const
Expand Down
4 changes: 2 additions & 2 deletions src/rt/typeinfo/ti_Aint.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@ class TypeInfo_Ai : TypeInfo_Array

override size_t getHash(in void* p) @trusted const
{
int[] s = *cast(int[]*)p;
return rt.util.hash.hashOf(s.ptr, s.length * int.sizeof);
const s = *cast(const int[]*)p;
return rt.util.hash.hashOf(s, 0);
}

override bool equals(in void* p1, in void* p2) const
Expand Down
4 changes: 2 additions & 2 deletions src/rt/typeinfo/ti_Along.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@ class TypeInfo_Al : TypeInfo_Array

override size_t getHash(in void* p) @trusted const
{
long[] s = *cast(long[]*)p;
return rt.util.hash.hashOf(s.ptr, s.length * long.sizeof);
const s = *cast(const long[]*)p;
return rt.util.hash.hashOf(s, 0);
}

override bool equals(in void* p1, in void* p2) const
Expand Down
4 changes: 2 additions & 2 deletions src/rt/typeinfo/ti_Ashort.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@ class TypeInfo_As : TypeInfo_Array

override size_t getHash(in void* p) @trusted const
{
short[] s = *cast(short[]*)p;
return rt.util.hash.hashOf(s.ptr, s.length * short.sizeof);
const s = *cast(const short[]*)p;
return rt.util.hash.hashOf(s, 0);
}

override bool equals(in void* p1, in void* p2) const
Expand Down
2 changes: 1 addition & 1 deletion src/rt/typeinfo/ti_cent.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ class TypeInfo_zi : TypeInfo

override size_t getHash(in void* p)
{
return rt.util.hash.hashOf(p, cent.sizeof);
return rt.util.hash.hashOf(p[0 .. cent.sizeof], 0);
}

override bool equals(in void* p1, in void* p2)
Expand Down
2 changes: 1 addition & 1 deletion src/rt/typeinfo/ti_delegate.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ class TypeInfo_D : TypeInfo

override size_t getHash(in void* p)
{
return rt.util.hash.hashOf(p, dg.sizeof);
return rt.util.hash.hashOf(p[0 .. dg.sizeof], 0);
}

override bool equals(in void* p1, in void* p2)
Expand Down
2 changes: 1 addition & 1 deletion src/rt/typeinfo/ti_long.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ class TypeInfo_l : TypeInfo

override size_t getHash(in void* p)
{
return rt.util.hash.hashOf(p, long.sizeof);
return rt.util.hash.hashOf(p[0 .. long.sizeof], 0);
}

override bool equals(in void* p1, in void* p2)
Expand Down
2 changes: 1 addition & 1 deletion src/rt/typeinfo/ti_ucent.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ class TypeInfo_zk : TypeInfo

override size_t getHash(in void* p)
{
return rt.util.hash.hashOf(p, ucent.sizeof);
return rt.util.hash.hashOf(p[0 .. ucent.sizeof], 0);
}

override bool equals(in void* p1, in void* p2)
Expand Down
2 changes: 1 addition & 1 deletion src/rt/typeinfo/ti_ulong.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ class TypeInfo_m : TypeInfo

override size_t getHash(in void* p)
{
return rt.util.hash.hashOf(p, ulong.sizeof);
return rt.util.hash.hashOf(p[0 .. ulong.sizeof]);
}

override bool equals(in void* p1, in void* p2)
Expand Down
6 changes: 3 additions & 3 deletions src/rt/util/container/hashtab.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,13 +144,13 @@ private:
return &p._value;
}

static hash_t hashOf(in ref Key key)
static hash_t hashOf(in ref Key key) @trusted
{
import rt.util.hash : hashOf;
static if (is(Key U : U[]))
return hashOf(cast(const ubyte*)key.ptr, key.length * key[0].sizeof);
return hashOf(key, 0);
else
return hashOf(cast(const ubyte*)&key, Key.sizeof);
return hashOf((&key)[0 .. 1], 0);
}

@property hash_t mask() const
Expand Down
14 changes: 7 additions & 7 deletions src/rt/util/hash.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,22 +17,22 @@ version( AnyX86 )
version = HasUnalignedOps;


@trusted pure nothrow
@trusted pure nothrow @nogc
size_t hashOf( const(void)[] buf, size_t seed = 0 )
{
return hashOf(buf.ptr, buf.length, seed);
}

@trusted pure nothrow
size_t hashOf( const(void)* buf, size_t len, size_t seed = 0 )
@system pure nothrow @nogc
size_t hashOf( const(void)* buf, size_t len, size_t seed )
Copy link
Member Author

@WalterBright WalterBright Jun 17, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get rid of the default argument, it just causes bugs

{
/*
* This is Paul Hsieh's SuperFastHash algorithm, described here:
* http://www.azillionmonkeys.com/qed/hash.html
* It is protected by the following open source license:
* http://www.azillionmonkeys.com/qed/weblicense.html
*/
static uint get16bits( const (ubyte)* x ) pure nothrow
static uint get16bits( const (ubyte)* x ) pure nothrow @nogc
{
// CTFE doesn't support casting ubyte* -> ushort*, so revert to
// per-byte access when in CTFE.
Expand Down Expand Up @@ -96,14 +96,14 @@ size_t hashOf( const(void)* buf, size_t len, size_t seed = 0 )
}

// Check that hashOf works with CTFE
unittest
@nogc nothrow pure unittest
{
size_t ctfeHash(string x)
{
return hashOf(x.ptr, x.length);
return hashOf(x.ptr, x.length, 0);
}

enum test_str = "Sample string";
enum size_t hashVal = ctfeHash(test_str);
assert(hashVal == hashOf(test_str.ptr, test_str.length));
assert(hashVal == hashOf(test_str.ptr, test_str.length, 0));
}
10 changes: 6 additions & 4 deletions src/rt/util/typeinfo.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,6 @@
*/
module rt.util.typeinfo;

public import rt.util.hash;

template Floating(T)
if (is(T == float) || is(T == double) || is(T == real))
{
Expand Down Expand Up @@ -42,7 +40,10 @@ if (is(T == float) || is(T == double) || is(T == real))
static if (is(T == float)) // special case?
return *cast(uint*)&value;
else
return rt.util.hash.hashOf(&value, T.sizeof);
{
import rt.util.hash;
return rt.util.hash.hashOf((&value)[0 .. 1], 0);
}
}
}
template Floating(T)
Expand Down Expand Up @@ -76,7 +77,8 @@ if (is(T == cfloat) || is(T == cdouble) || is(T == creal))
{
if (value == 0 + 0i)
value = 0 + 0i;
return rt.util.hash.hashOf(&value, T.sizeof);
import rt.util.hash;
return rt.util.hash.hashOf((&value)[0 .. 1], 0);
}
}

Expand Down