Couple of code fixes #7731
Couple of code fixes #7731
Conversation
|
CC @dotnet/roslyn-compiler, @dotnet/roslyn-ide A code analysis utility was run against the Roslyn repo and filed as #7677. I did a quick sweep through the issues noted and fixed up a number of them in this PR. The type of changes fixed were:
|
| @@ -274,13 +274,6 @@ internal static bool ReportConflictWithParameter(Symbol parameter, Symbol newSym | |||
| return false; | |||
| } | |||
|
|
|||
| if (newSymbolKind == SymbolKind.Parameter || newSymbolKind == SymbolKind.Local) | |||
There was a problem hiding this comment.
Expand the review up a few lines and you'll see an identical check being made which unconditionally returns in the if body. Hence this code path should never get hit.
There was a problem hiding this comment.
Any sense how this snuck in? Merge issue?
There was a problem hiding this comment.
It was done as a part of a single commit. It's possible there was a local merge issue that caused this though.
|
Updated to respond to PR feedback. |
|
LGTM |
2 similar comments
|
LGTM |
|
LGTM |
| thread2.Start(); | ||
| thread1.Start(); |
There was a problem hiding this comment.
Read the context. The purpose of the enclosing if statement is to permute the order in which the threads are started.
There was a problem hiding this comment.
The OS makes no guarantee that the threads run in the order started, so why pretend?
There was a problem hiding this comment.
Because it helps test for nondeterministic behavior.
|
Anyone from @dotnet/roslyn-ide able to sign off on the IDE portion of the change? |
| @@ -161,7 +169,7 @@ private void ObjectEquals3() | |||
| var allValues = _equalityUnits.SelectMany(x => x.AllValues); | |||
| foreach (var value in allValues) | |||
| { | |||
| Assert.NotEqual((object)42, value); | |||
| Assert.False(value.Equals((object)42)); | |||
There was a problem hiding this comment.
Why the change? Just to make sure you don't get surprised by xunit behavior?
There was a problem hiding this comment.
@jasonmalinowski the intent of this check was to verify the behavior of object.Equals(object). It seemed wrong to rely on this being tested as an implementation detail of Assert.NotEqual (and in fact it doesn't guarantee this). Much more direct to just call it directly.
|
👍. Also, please either edit the "PR Feedback" commit with a proper description of the change, or just squash it. |
Corrects a couple of items noted in dotnet#7677
|
@jasonmalinowski will do on the squash. During the active review of a PR I generally avoid rebasing if possible. This helps keep the comments relevant and seems to generally let things flow better. Once it gets signed off I squash, push, wait for Jenknis and merge. |
Corrects a couple of items noted in #7677