Skip to content
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

Merged
merged 1 commit into from
Feb 11, 2015

Conversation

pdelvo
Copy link
Contributor

@pdelvo pdelvo commented Feb 3, 2015

Fixes #57.

I hope it is okay that I just went ahead and fixed it.

@pdelvo
Copy link
Contributor Author

pdelvo commented Feb 3, 2015

Btw. Why are the additional locations not used to determine equality?

@theoy theoy added Concept-API This issue involves adding, removing, clarification, or modification of an API. Area-Analyzers Area-Compilers Bug labels Feb 4, 2015
@theoy
Copy link
Contributor

theoy commented Feb 4, 2015

I hope it is okay that I just went ahead and fixed it.
-- @pdelvo

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.

@pdelvo
Copy link
Contributor Author

pdelvo commented Feb 6, 2015

@AdamSpeight2008
I don't think that the foreach is expensive. foeach on Arrays are getting turned into a for loop by the compiler if Im not mistakten.

@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 new[] {null, "foo"} would have the same hash as new[] { "foo"}

@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;
Copy link
Contributor

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.

@pdelvo
Copy link
Contributor Author

pdelvo commented Feb 6, 2015

@pharring I fixed that and I fixed a possible NRE if the location is null

@AdamSpeight2008
Copy link
Contributor

Doesn't foreach get converted into a while loop?

var en = foos.GetEnumerator();
while( en.MoveNext() )
{

}

@pdelvo
Copy link
Contributor Author

pdelvo commented Feb 6, 2015

I can't compile roslyn on my machine this is why this stupid mistake could happen >.<
@AdamSpeight2008 Roslyn does optimize that.

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);
Copy link
Contributor

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.

@pdelvo
Copy link
Contributor Author

pdelvo commented Feb 6, 2015

I hope this is what you are looking for. If it is Im going to squash these commits together.

@pdelvo
Copy link
Contributor Author

pdelvo commented Feb 6, 2015

I squashed this into one commit.

@pdelvo
Copy link
Contributor Author

pdelvo commented Feb 6, 2015

The failed test semms to be unrelated to this

@mavasani
Copy link
Contributor

@pdelvo Agreed, failing test "HandleSolutionProjectTypeSolutionFolder" seems to be unrelated to this change. I'll merge your PR. Thanks again!

mavasani added a commit that referenced this pull request Feb 11, 2015
Fix SimpleDiagnostic GetHashCode is more specific than Equals
@mavasani mavasani merged commit 142828d into dotnet:master Feb 11, 2015
@jaredpar
Copy link
Member

@pdelvo

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.

@pdelvo
Copy link
Contributor Author

pdelvo commented Feb 11, 2015

Done

@jaredpar
Copy link
Member

@pdelvo thank you!

@mavasani mavasani removed their assignment Apr 14, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Analyzers Area-Compilers Bug Concept-API This issue involves adding, removing, clarification, or modification of an API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SimpleDiagnostic GetHashCode is inconsistent
7 participants