[Add] IEquatable to WitnessCondition
#3572
Conversation
cschuchardt88
added
Feature
cschuchardt88
added
the
Blocker
shargon
removed
the
Blocker
…huchardt88/neo into fix/equals-witnessconditions
| { | ||
| if (ReferenceEquals(this, other)) | ||
| return true; | ||
| if (other is null) return false; |
There was a problem hiding this comment.
null check before reference equal?
There was a problem hiding this comment.
no, because of
AndCondition a1 = null;
ReferenceEquals(a1, a1) = true;
a1.Equals(a1) = true;
There was a problem hiding this comment.
I don't understand your example, I think @Jim8y is right, if we will check null reference, it's better to do it in the first line
There was a problem hiding this comment.
So it should be this way? there are implementations in other classes that have the different order, should them be updated as well?
There was a problem hiding this comment.
Well this actually wont matter, not really affect the functionality of neo.
| return true; | ||
| if (other is null) return false; | ||
| return Type == other.Type && | ||
| Size == other.Size && |
There was a problem hiding this comment.
should this also be updated to is ?
|
LGTM, minor potential issues being commented. |
| Size == other.Size; | ||
| return | ||
| Type == other.Type && | ||
| Expression.Equals(other.Expression); |
There was a problem hiding this comment.
We should review this test, because it was wrong
Change Log
IEquatabletoWitnessCondition,AndCondition,BooleanCondition,CalledByContractCondition,CalledByEntryCondition,CalledByGroupCondition,GroupCondition,NotCondition,OrConditionandScriptHashConditionclassesIEquatableonWitnessCondition,AndCondition,BooleanCondition,CalledByContractCondition,CalledByEntryCondition,CalledByGroupCondition,GroupCondition,NotCondition,OrConditionandScriptHashConditionclassesType of change
How Has This Been Tested?
Test Configuration:
Checklist: