-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Remove mutable state from rename #65848
Conversation
@@ -118,6 +114,8 @@ public async Task<MutableConflictResolution> ResolveConflictsAsync() | |||
var intermediateSolution = conflictResolution.OldSolution; | |||
foreach (var documentsByProject in documentsGroupedByTopologicallySortedProjectId) | |||
{ | |||
var conflictLocations = SpecializedCollections.EmptySet<ConflictLocationInfo>(); |
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.
conflict locations is fresh for each project we look at. we know that because in phase-0, we always assign over whatever the prior project computed. so this just makes things simpler here.
{ | ||
try | ||
{ | ||
var documentIds = new HashSet<DocumentId>(); |
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.
Is it faster to use an ImmutableHashSet builder here?
{ | ||
try | ||
{ | ||
var documentIds = new HashSet<DocumentId>(); | ||
var possibleNameConflictsList = new List<string>(); |
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.
Ditto: immutable array builder?
Was finding these mutable fields very confusing for determining what was going on in rename as i work on shared syntax trees. So pulled out this PR to clean it up.