Fix Equal() and GetHashCode() #332
Conversation
|
@ixje Please check this. |
| public override bool Equals(object obj) | ||
| { | ||
| return this == obj; | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@shargon What do you think?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
is @shargon afk or did he share his opinion elsewhere?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| return hash; | ||
| } | ||
| } | ||
|
|
||
| public override bool ToBoolean() | ||
| { | ||
| return true; |
There was a problem hiding this comment.
| return true; | |
| if (Size > Integer.MaxSize) return true; | |
| return Unsafe.NotZero(InnerBuffer); |
same logic as applied for ByteString.
There was a problem hiding this comment.
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
Closes #330