-
Notifications
You must be signed in to change notification settings - Fork 4.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix SimpleDiagnostic GetHashCode is more specific than Equals #209
Conversation
Btw. Why are the additional locations not used to determine equality? |
It is absolutely okay, in fact, awesome. Thanks for offering a fix - we're almost ready to start blasting through pull requests. We're just in the process of replacing our source control internally with Git on both sides so that we can fast-track all the subsequent pull requests 😄 Assignining to @mavasani since he has the corresponding bug too. |
@AdamSpeight2008 @mavasani Create is calling the constructor which is checking messageArgs for null. It is using an empty array instead if it is. Hash.Combine is checking for null so this should not be a problem if items in messageArgs are null. If I ignore null items in the array @pharring I did not know about Hash.CombineValues. Is the jit compiling the foreach loop in there into a for loop too? I came up with a solution which should be faster and more readable. |
Hash.Combine(_location.GetHashCode(), | ||
Hash.Combine(_severity.GetHashCode(), _warningLevel) | ||
))); | ||
int hash = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Assignment to zero is not necessary since it's going to be overwritten on the next line.
@pharring I fixed that and I fixed a possible NRE if the location is null |
Doesn't
|
I can't compile roslyn on my machine this is why this stupid mistake could happen >.< Edit: Im going to squash that into one commit when this is okay |
hash = Hash.Combine((int)_severity, hash); | ||
hash = Hash.Combine(_warningLevel, hash); | ||
hash = Hash.Combine(Hash.CombineValues(_messageArgs), hash); | ||
hash = Hash.Combine(_descriptor, hash); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just an observation: While using the local variable undoubtedly makes the code easier to read, it does introduce a slight performance overhead. The original form's IL is shorter and (obviously) eliminates a local variable slot. The Roslyn compiler (currently) doesn't try to eliminate the local. I don't know whether the various JIT compilers will do so either.
I hope this is what you are looking for. If it is Im going to squash these commits together. |
I squashed this into one commit. |
The failed test semms to be unrelated to this |
@pdelvo Agreed, failing test "HandleSolutionProjectTypeSolutionFolder" seems to be unrelated to this change. I'll merge your PR. Thanks again! |
Fix SimpleDiagnostic GetHashCode is more specific than Equals
Could you take a minute and sign the .NET Foundation CLA for the contributed code? https://cla2.dotnetfoundation.org/ This should have been done before we merged the PR but there was some confusion on the process. |
Done |
@pdelvo thank you! |
Fixes #57.
I hope it is okay that I just went ahead and fixed it.