Use ReadOnlyCollection.Empty #10013
Use ReadOnlyCollection.Empty #10013
Conversation
dotnet-policy-service
bot
requested review from
dipeshmsft and
singhashish-wpf
dotnet-policy-service
bot
added
PR
| @@ -1868,7 +1868,7 @@ protected virtual IEnumerator<IInputElement> HostedElementsCore | |||
| if (_complexContent == null || !(_complexContent.TextContainer is TextContainer)) | |||
| { | |||
| // Return empty collection | |||
| return new HostedElements(new ReadOnlyCollection<TextSegment>(new List<TextSegment>(0))); | |||
| return new HostedElements(ReadOnlyCollection<TextSegment>.Empty); | |||
There was a problem hiding this comment.
@ThomasGoulet73 Could you revert the changes in Textblock? #9993
Thank you.
There was a problem hiding this comment.
Sorry I missed your comment when answering below. Since it's the same changes it shouldn't conflict, I think it's fine for both PRs to stay as-is.
There was a problem hiding this comment.
I have asked because I personally try to not do overlapping changes (even if they would be the same) with other contributors open PRs when I'm aware of them existing (usually that includes search in PRs since I've got burnt on your PR many months ago, and this one would probably would go with "ReadOnlyCollection" search.). The other time you've asked me, now I'm politely asking you.
Not trying to be difficult, you may resolve the conversation as you see fit, it was just an ask since my changes were the driving subject of that PR and now it feels a bit wasted if it's already contained here.
There was a problem hiding this comment.
Sorry, I didn't notice that it was one of the main points of your PR. I thought the main point of your PR was using Span and the ReadOnlyCollection change was just a "While you're there" change. It wouldn't conflict because it's the same change but I'll revert it because I don't want to "erase" your work if my PR is merged first.
| @@ -224,7 +224,7 @@ internal IEnumerable<IManipulator> GetManipulatorsReadOnly() | |||
| } | |||
| else | |||
| { | |||
| return new ReadOnlyCollection<IManipulator>(new List<IManipulator>(2)); | |||
There was a problem hiding this comment.
I didn't checked the whole code but unlike all of the other changes, this original line is allocating a list with a size of 2 and yours is empty. Can this become a problem?
There was a problem hiding this comment.
I don't believe this is a problem, the 2 here is for the initial capacity of the list which doesn't affect the size and thus doesn't affect the ReadOnlyCollection. Here's a small test:
https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEUCuA7AHwAEAmABgFgAoUgRmqLIAIjaUBuB51gOgGEIAG0EwwGAJYQ8AZx4B5YACtRGALIQAJjEGcqDWgE4AFHhgB3JgCUYAQw1y8ggJ4DhKyXgA8EJSoB8JuZMADLi0hjevmIBJACUsfwQ+BixuqzG1nYOzq4iYh6RytE8AKIAtgAOGE6JyalAA
There was a problem hiding this comment.
Yeah, this ain't an issue unless you're doing dark magic with the wrapped collection. It was just a wasted memory to begin with.
Description
Replaces ReadOnlyCollection allocations with a shared singleton. I also replaced EmptyList with ReadOnlyCollection.Empty because it does the same thing, is a BCL API and allows to potentially allocate less ReadOnlyCollection instances.
Customer Impact
Better perf and memory usage.
Regression
None.
Testing
Local testing.
Risk
Low.