Skip to content
This repository has been archived by the owner on Nov 22, 2023. It is now read-only.

Fix Equal() and GetHashCode() #332

Merged
merged 3 commits into from
Jun 15, 2020
Merged

Fix Equal() and GetHashCode() #332

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e65ad42
Fix Equal() and GetHashCode()
erikzhang Jun 5, 2020
028ccf2
Fix Buffer
erikzhang Jun 5, 2020
c5b9718
IEquatable
erikzhang Jun 5, 2020
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
9 changes: 7 additions & 2 deletions src/neo-vm/Types/Boolean.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,16 @@ public Boolean(bool value)
this.value = value;
}

public override bool Equals(PrimitiveType other)
public override bool Equals(StackItem other)
{
if (ReferenceEquals(this, other)) return true;
if (other is Boolean b) return value == b.value;
return base.Equals(other);
return false;
}

public override int GetHashCode()
{
return HashCode.Combine(value);
}

public override BigInteger ToBigInteger()
Expand Down
16 changes: 0 additions & 16 deletions src/neo-vm/Types/Buffer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,22 +40,6 @@ public override StackItem ConvertTo(StackItemType type)
}
}

public override bool Equals(object obj)
{
return this == obj;
}
Comment on lines -43 to -46
Copy link
Contributor

@ixje ixje Jun 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Instead of deleting should we do reference + sequence equality checking (=the same as ByteString)? Now it is only reference equality checked via StackItem.

I can imagine a use-case where you dynamically build 2 Buffers, then compare if they're equal (e.g. for some validation scheme). In order to do that you would have to convert them to ByteStrings first or the results will be false.

Copy link
Member Author

Choose a reason for hiding this comment

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

@shargon What do you think?

Copy link
Member Author

@erikzhang erikzhang Jun 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

We are temporarily unable to reach a consensus on this issue, and the function here has not been modified, it is still the same as the original. So I will merge this PR first. If there is a problem, you can create a new issue separately. @ixje

Copy link
Contributor

Choose a reason for hiding this comment

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

is @shargon afk or did he share his opinion elsewhere?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry i miss this conversation.

reference + sequence equality checking

If it's the same reference, the sequence will be the same, but reference it's good for me

Copy link
Contributor

Choose a reason for hiding this comment

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

@shargon The point I'm trying to make is that the immutable version (ByteString) does reference + sequence checking, but the mutable version Buffer does not. Which I think should be the same, and you previous

Copy link
Member

Choose a reason for hiding this comment

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

It could be easier, incompatible with c# but easy for developers. Use == and check if the buffer it's the same. But also... do you want to be able to compare ByteString with Buffer?

Copy link
Contributor

Choose a reason for hiding this comment

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

as described above I can imagine a case where you'd like to compare 2 Buffers on sequence without having to convert them to ByteString first. It doesn't sound logical to me that that sequence comparison only works in the immutable variant.


public override int GetHashCode()
{
unchecked
{
int hash = 17;
foreach (byte element in InnerBuffer)
hash = hash * 31 + element;
return hash;
}
}

public override bool ToBoolean()
{
return true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return true;
if (Size > Integer.MaxSize) return true;
return Unsafe.NotZero(InnerBuffer);

same logic as applied for ByteString.

Copy link
Member Author

Choose a reason for hiding this comment

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

We are temporarily unable to reach a consensus on this issue, and the function here has not been modified, it is still the same as the original. So I will merge this PR first. If there is a problem, you can create a new issue separately. @ixje

Expand Down
18 changes: 18 additions & 0 deletions src/neo-vm/Types/ByteString.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,24 @@ public ByteString(ReadOnlyMemory<byte> value)
this.Memory = value;
}

public override bool Equals(StackItem other)
{
if (ReferenceEquals(this, other)) return true;
if (other is ByteString b) return Span.SequenceEqual(b.Span);
return false;
}

public override int GetHashCode()
{
unchecked
{
erikzhang marked this conversation as resolved.
Show resolved Hide resolved
int hash = 17;
foreach (byte element in Span)
hash = hash * 31 + element;
return hash;
}
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static implicit operator ReadOnlyMemory<byte>(ByteString value)
{
Expand Down
5 changes: 0 additions & 5 deletions src/neo-vm/Types/CompoundType.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,6 @@ protected CompoundType(ReferenceCounter referenceCounter)

public abstract void Clear();

public override bool Equals(object obj)
{
return ReferenceEquals(this, obj);
}

public override int GetHashCode()
{
throw new NotSupportedException();
Expand Down
9 changes: 7 additions & 2 deletions src/neo-vm/Types/Integer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,16 @@ public Integer(BigInteger value)
this.value = value;
}

public override bool Equals(PrimitiveType other)
public override bool Equals(StackItem other)
{
if (ReferenceEquals(this, other)) return true;
if (other is Integer i) return value == i.value;
return base.Equals(other);
return false;
}

public override int GetHashCode()
{
return HashCode.Combine(value);
}

public override BigInteger ToBigInteger()
Expand Down
8 changes: 4 additions & 4 deletions src/neo-vm/Types/InteropInterface.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,16 +15,16 @@ public InteropInterface(object value)
_object = value ?? throw new ArgumentException();
}

public override bool Equals(object obj)
public override bool Equals(StackItem other)
{
if (ReferenceEquals(this, obj)) return true;
if (obj is InteropInterface i) return _object.Equals(i._object);
if (ReferenceEquals(this, other)) return true;
if (other is InteropInterface i) return _object.Equals(i._object);
shargon marked this conversation as resolved.
Show resolved Hide resolved
return false;
}

public override int GetHashCode()
{
throw new NotSupportedException();
return HashCode.Combine(_object);
}

public T GetInterface<T>()
Expand Down
10 changes: 4 additions & 6 deletions src/neo-vm/Types/Null.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,17 +15,15 @@ public override StackItem ConvertTo(StackItemType type)
return this;
}

public override bool Equals(object obj)
public override bool Equals(StackItem other)
{
if (ReferenceEquals(this, obj)) return true;
if (obj is null) return true;
if (obj is Null) return true;
return false;
if (ReferenceEquals(this, other)) return true;
return other is Null;
}

public override int GetHashCode()
{
throw new NotSupportedException();
return 0;
}

public override bool ToBoolean()
Expand Down
9 changes: 5 additions & 4 deletions src/neo-vm/Types/Pointer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
using System;
using System.Diagnostics;

namespace Neo.VM.Types
Expand All @@ -15,16 +16,16 @@ public Pointer(Script script, int position)
this.Position = position;
}

public override bool Equals(object obj)
public override bool Equals(StackItem other)
{
if (obj == this) return true;
if (obj is Pointer p) return Position == p.Position && Script.Equals(p.Script);
if (other == this) return true;
if (other is Pointer p) return Position == p.Position && Script.Equals(p.Script);
return false;
}

public override int GetHashCode()
{
return Position.GetHashCode() + (31 * Script.GetHashCode());
return HashCode.Combine(Script, Position);
}

public override bool ToBoolean()
Expand Down
24 changes: 2 additions & 22 deletions src/neo-vm/Types/PrimitiveType.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,29 +22,9 @@ public override StackItem ConvertTo(StackItemType type)
};
}

public sealed override bool Equals(object obj)
{
if (ReferenceEquals(this, obj)) return true;
if (obj is PrimitiveType p) return Equals(p);
return false;
}

public virtual bool Equals(PrimitiveType other)
{
if (ReferenceEquals(this, other)) return true;
return Span.SequenceEqual(other.Span);
}
public abstract override bool Equals(StackItem other);

public sealed override int GetHashCode()
{
unchecked
{
int hash = 17;
foreach (byte element in Span)
hash = hash * 31 + element;
return hash;
}
}
public abstract override int GetHashCode();

public virtual BigInteger ToBigInteger()
{
Expand Down
18 changes: 14 additions & 4 deletions src/neo-vm/Types/StackItem.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
#pragma warning disable CS0659

using System;
using System.Numerics;
using System.Runtime.CompilerServices;

namespace Neo.VM.Types
{
public abstract class StackItem
public abstract class StackItem : IEquatable<StackItem>
{
public static StackItem False { get; } = new Boolean(false);
public bool IsNull => this is Null;
Expand All @@ -19,16 +21,24 @@ public virtual StackItem ConvertTo(StackItemType type)
throw new InvalidCastException();
}

public abstract override bool Equals(object obj);
public sealed override bool Equals(object obj)
erikzhang marked this conversation as resolved.
Show resolved Hide resolved
{
if (ReferenceEquals(this, obj)) return true;
if (obj is StackItem item) return Equals(item);
return false;
}

public virtual bool Equals(StackItem other)
{
return ReferenceEquals(this, other);
}

public static StackItem FromInterface(object value)
{
if (value is null) return Null;
return new InteropInterface(value);
}

public abstract override int GetHashCode();

public abstract bool ToBoolean();

[MethodImpl(MethodImplOptions.AggressiveInlining)]
Expand Down
12 changes: 3 additions & 9 deletions src/neo-vm/Types/Struct.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
using System;
using System.Collections.Generic;

namespace Neo.VM.Types
Expand Down Expand Up @@ -52,13 +51,13 @@ public override StackItem ConvertTo(StackItemType type)
return base.ConvertTo(type);
}

public override bool Equals(object obj)
public override bool Equals(StackItem other)
{
if (!(obj is Struct other)) return false;
if (!(other is Struct s)) return false;
erikzhang marked this conversation as resolved.
Show resolved Hide resolved
Stack<StackItem> stack1 = new Stack<StackItem>();
Stack<StackItem> stack2 = new Stack<StackItem>();
stack1.Push(this);
stack2.Push(other);
stack2.Push(s);
while (stack1.Count > 0)
{
StackItem a = stack1.Pop();
Expand All @@ -80,10 +79,5 @@ public override bool Equals(object obj)
}
return true;
}

public override int GetHashCode()
{
throw new NotSupportedException();
}
}
}
10 changes: 5 additions & 5 deletions tests/neo-vm.Tests/UtStackItem.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,8 @@ public void HashCodeTest()

itemA = new VM.Types.Buffer(1);
itemB = new VM.Types.Buffer(1);
itemC = new VM.Types.Buffer(123);

Assert.IsTrue(itemA.GetHashCode() == itemB.GetHashCode());
Assert.IsTrue(itemA.GetHashCode() != itemC.GetHashCode());
Assert.IsTrue(itemA.GetHashCode() != itemB.GetHashCode());

itemA = true;
itemB = true;
Expand All @@ -41,8 +39,9 @@ public void HashCodeTest()
Assert.IsTrue(itemA.GetHashCode() != itemC.GetHashCode());

itemA = new Null();
itemB = new Null();

Assert.ThrowsException<NotSupportedException>(() => itemA.GetHashCode());
Assert.IsTrue(itemA.GetHashCode() == itemB.GetHashCode());

itemA = new VM.Types.Array();

Expand All @@ -57,8 +56,9 @@ public void HashCodeTest()
Assert.ThrowsException<NotSupportedException>(() => itemA.GetHashCode());

itemA = new InteropInterface(123);
itemB = new InteropInterface(123);

Assert.ThrowsException<NotSupportedException>(() => itemA.GetHashCode());
Assert.IsTrue(itemA.GetHashCode() == itemB.GetHashCode());

var script = new Script(new byte[0]);
itemA = new Pointer(script, 123);
Expand Down