PR feedback on syncing. #72996
PR feedback on syncing. #72996
Conversation
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
| @@ -152,6 +149,18 @@ async Task<ImmutableArray<DocumentInfo>> CreateDocumentInfosAsync(DocumentChecks | |||
| // TODO: do we need version? | |||
| return new DocumentInfo(attributes, textLoader, documentServiceProvider: null); | |||
| } | |||
|
|
|||
| public AssetHelper<T> GetAssetHelper<T>() | |||
There was a problem hiding this comment.
this pattern helped make the callsites work better with inference, by splitting the type args into the parts that have to be specified (the T) and the part that can be inferred (the TArgs).
|
@ToddGrun ptal. |
src/Workspaces/Core/Portable/Workspace/Solution/TextDocumentStates.cs
Outdated
Show resolved
Hide resolved
| @@ -152,6 +149,18 @@ async Task<ImmutableArray<DocumentInfo>> CreateDocumentInfosAsync(DocumentChecks | |||
| // TODO: do we need version? | |||
There was a problem hiding this comment.
want to talk to you in person about this.
There was a problem hiding this comment.
Might be a week or two before I'm back in the office due to that machine going down. I don't have any strong thoughts here, I just saw a todo and wondered if it was still valid.
| assetPath, checksumSet, | ||
| static (checksum, asset, builder) => builder.Add(asset), | ||
| builder, | ||
| cancellationToken).ConfigureAwait(false); | ||
|
|
||
| return builder.ToImmutableAndClear(); | ||
| return builder.MoveToImmutable(); |
There was a problem hiding this comment.
@ToddGrun note the movetoimmutable here. the builder itself produces no internal garbage. it just holds onto the array and count. MoveToImmtuable is just an ownership transform of its array to the IA result. So the only thing left is the builder itself, which should be trivially collected.
Note: one pattern we could consider here is pooling these builders when we've successfully moved their internal arrays out.
src/Workspaces/Remote/ServiceHub/Host/RemoteWorkspace.SolutionCreator.cs
Outdated
Show resolved
Hide resolved
|
@jasonmalinowski For review when you get back. |
Followup to #72990.