Conversation
|
|
||
| return ReferenceEquals(this.OriginalDefinition, other.OriginalDefinition) && | ||
| TypeSymbol.Equals(ContainingType, other.ContainingType, compareKind) && | ||
| TypeSymbol.Equals(Type, other.Type, compareKind); |
There was a problem hiding this comment.
TypeSymbol.Equals(Type, other.Type, compareKind) [](start = 19, length = 48)
This condition feels unnecessary.
| // PERF: VSadov specifically replaced the short-circuited operators in changeset #24717. | ||
| if (selfIsDeclaration | otherIsDeclaration) | ||
| { | ||
| return selfIsDeclaration & otherIsDeclaration; |
There was a problem hiding this comment.
return selfIsDeclaration & otherIsDeclaration; [](start = 16, length = 46)
This doesn't look right. The methods could be equal even when only one of them IsConstructedFrom
| // substituted in the same way. | ||
| if (!TypeSymbol.Equals(this.ContainingType, other.ContainingType, compareKind)) return false; | ||
|
|
||
| // If both are declarations, then we don't need to check type arguments |
There was a problem hiding this comment.
declarations [](start = 27, length = 12)
The term "declaration" is somewhat confusing here. Consider using "not constructed" instead. Same for variable names.
| int arity = this.Arity; | ||
| for (int i = 0; i < arity; i++) | ||
| { | ||
| if (!this.TypeArgumentsWithAnnotations[i].Equals(other.TypeArgumentsWithAnnotations[i], compareKind)) |
There was a problem hiding this comment.
this.TypeArgumentsWithAnnotations [](start = 21, length = 33)
Consider caching this value in a local.
| return visitor.VisitField(this); | ||
| } | ||
|
|
||
| public override bool Equals(Symbol obj, TypeCompareKind compareKind) |
There was a problem hiding this comment.
override [](start = 15, length = 8)
sealed?
| Assert.Equal(type1.GetHashCode(), type2.GetHashCode()); | ||
| Assert.Equal(SymbolEqualityComparer.Default.GetHashCode(type1), SymbolEqualityComparer.Default.GetHashCode(type2)); | ||
| Assert.Equal(SymbolEqualityComparer.IncludeNullability.GetHashCode(type1), SymbolEqualityComparer.IncludeNullability.GetHashCode(type2)); | ||
| if (expectHashCodeMatch) |
There was a problem hiding this comment.
expectHashCodeMatch [](start = 16, length = 19)
Why is this conditional on a separate flag? If Object.Equals(Object) returns true, the hash code must match, so this should be conditioned on expectedDefault.
| var methodDecl = root.DescendantNodes().OfType<MethodDeclarationSyntax>().Single(); | ||
| var methodBlockOperation = model.GetOperation(methodDecl); | ||
| var fieldReferenceOperation = methodBlockOperation.Descendants().OfType<IFieldReferenceOperation>().Single(); | ||
| Assert.True(fieldSym.Equals(fieldReferenceOperation.Field)); |
There was a problem hiding this comment.
Assert.True(fieldSym.Equals(fieldReferenceOperation.Field)); [](start = 16, length = 60)
Please Assert that hash code is equal as well
| ); | ||
| } | ||
|
|
||
| private void VerifyEquality(ISymbol type1, ISymbol type2, bool expectedDefault, bool expectedIncludeNullability, bool expectHashCodeMatch = true) |
There was a problem hiding this comment.
type1 [](start = 44, length = 5)
Please rename to symbol1 and the other to symbol2.
| expectedDefault: true, | ||
| expectedIncludeNullability: false | ||
| ); | ||
| } |
There was a problem hiding this comment.
} [](start = 8, length = 1)
It looks like we are lacking tests for generic methods, matrix [(not constructed, constructed) method] * [(not constructed, constructed) containing type].
| return visitor.VisitField(this); | ||
| } | ||
|
|
||
| public override bool Equals(Symbol obj, TypeCompareKind compareKind) |
There was a problem hiding this comment.
Equals [](start = 29, length = 6)
What about events and properties?
|
Done with review pass (iteration 2) |
|
Thanks @AlekseyTs. this isn't ready for review yet, but i'll keep the comments in mind as I update it |
No description provided.