Switch to ToImmutableAndClear #73010
Conversation
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
|
@ToddGrun ptal. |
...nstructorParametersFromMembers/AddConstructorParametersFromMembersCodeRefactoringProvider.cs
Outdated
Show resolved
Hide resolved
src/Features/Core/Portable/AddFileBanner/AbstractAddFileBannerCodeRefactoringProvider.cs
Outdated
Show resolved
Hide resolved
src/Features/Core/Portable/AddImport/AbstractAddImportFeatureService.cs
Outdated
Show resolved
Hide resolved
src/Features/Core/Portable/ChangeSignature/AbstractChangeSignatureService.cs
Outdated
Show resolved
Hide resolved
src/Features/Core/Portable/CodeFixes/Configuration/ConfigurationUpdater.cs
Outdated
Show resolved
Hide resolved
...table/Completion/Providers/ImportCompletionProvider/ExtensionMethodImportCompletionHelper.cs
Outdated
Show resolved
Hide resolved
...ures/Core/Portable/Completion/Providers/Scripting/AbstractLoadDirectiveCompletionProvider.cs
Show resolved
Hide resolved
src/Features/Core/Portable/EditAndContinue/Remote/RemoteEditAndContinueServiceProxy.cs
Outdated
Show resolved
Hide resolved
...erateConstructorFromMembers/AbstractGenerateConstructorFromMembersCodeRefactoringProvider.cs
Show resolved
Hide resolved
...tures/Core/Portable/GenerateMember/GenerateConstructor/AbstractGenerateConstructorService.cs
Show resolved
Hide resolved
...e/Portable/GenerateMember/GenerateParameterizedMember/AbstractGenerateMethodService.State.cs
Show resolved
Hide resolved
...mber/GenerateParameterizedMember/AbstractGenerateParameterizedMemberService.SignatureInfo.cs
Outdated
Show resolved
Hide resolved
src/Features/Core/Portable/InheritanceMargin/AbstractInheritanceMarginService_Helpers.cs
Show resolved
Hide resolved
src/Features/Core/Portable/PdbSourceDocument/DocumentDebugInfoReader.cs
Outdated
Show resolved
Hide resolved
src/Features/Core/Portable/Wrapping/BinaryExpression/BinaryExpressionCodeActionComputer.cs
Outdated
Show resolved
Hide resolved
src/Features/CSharp/Portable/BraceCompletion/AbstractCurlyBraceOrBracketCompletionService.cs
Outdated
Show resolved
Hide resolved
src/Features/CSharp/Portable/ConvertProgram/ConvertProgramTransform_TopLevelStatements.cs
Outdated
Show resolved
Hide resolved
...table/GenerateMember/GenerateParameterizedMember/CSharpGenerateParameterizedMemberService.cs
Outdated
Show resolved
Hide resolved
...table/GenerateMember/GenerateParameterizedMember/CSharpGenerateParameterizedMemberService.cs
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| using var statements = TemporaryArray<SyntaxNode>.Empty; |
There was a problem hiding this comment.
I hesitated making suggestions to switch to TemporaryArray in non-trivial codepaths, as it seems like it would be easy for the code to end up adding a couple more entries and no longer being in a happy path.
What are your thoughts on adding a debug only assert into TemporaryArray in the case where it ends up exceeding the happy path size threshold to track down misuses of that class?
There was a problem hiding this comment.
we should talk to sam about that, and do in another PR :)
imo, i'm wary abotu it. because maybe you have a case that is 1 elements 99% of the time, but very rarely you go up to >4. TempArray is great for that purpose.
There was a problem hiding this comment.
I guess my thought is that if I see a loop in the code, TemporaryArray is essentially not a good choice. I feel like those cases can just ArrayBuilder or FixedSizeArrayBuilder and work almost as well. I just feel like TemporaryArray is too easy to move off the happy path without realizing it, and if I have to give up that case you mentioned to not have to worry about that, I'm ok with that.
There was a problem hiding this comment.
there is no loop here. this is 'CreateEqualsMethod' which is either 1, 2, or 3 statemnets depending on the form we're ccreating.
There was a problem hiding this comment.
I hesitated making suggestions to switch to TemporaryArray in non-trivial codepaths,
i agree with that. i only switched to TempArray where i thought it was trivial. :)
There was a problem hiding this comment.
I think our thoughts on this code being trivial differ :) .
There was a problem hiding this comment.
fair enough. i thought it was trivial here, likely because i know what thsi feature does well :)
ToImmutableAndClear is better than ToImmutable when the builder isn't needed any more. It will allow moving of the underlying array if the count and capacity is the same, avoiding a copy.