Remove LINQ usage for IEnumerable comparisons #57323
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ghost
added
the
needs-area-label
captainsafia
added
area-mvc
3 tasks
JamesNK
reviewed
Aug 13, 2024
| return x.PropertyName == y.PropertyName && | ||
| x.Mapping.Count == y.Mapping.Count && | ||
| x.Mapping.Keys.All(key => y.Mapping.TryGetValue(key, out var yValue) && x.Mapping[key] == yValue); | ||
| ComparerHelpers.DictionaryEquals((Dictionary<string, string>)x.Mapping, (Dictionary<string, string>)y.Mapping, StringComparer.Ordinal); |
Member
There was a problem hiding this comment.
I don't think these should be casted without checking the type. OpenAPI library automatically creates Dictionary and List instances for these properties, but someone could set their own IDictionary implementation.
Member
Author
There was a problem hiding this comment.
Yep, I shared this insight over in the other PR. I was optimizing for code brevity here given how unlikely it is for these to be overridden with non-default implementations but I can adjust if we want things to be a little sturdies here.
Member
There was a problem hiding this comment.
I think making ListEquals and DictionaryEquals a little bigger to handle other types is worth the safety. Also, it allows the casts to be removed when calling them.
JamesNK
approved these changes
Aug 14, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Contributes towards #56829. Refresh of #56599.
Enumerable.Allwith a custom implementation ofDictionaryEqualsandListEqualsto avoid overhead due to lambda capture in existing comparers.DictionaryEqualsandListEqualsto avoid interface dispatch overhead. See Remove LINQ usage from OpenAPI comparers #56599 (comment) fore more info.Resulting benchmarks from this change are as follows:
Before
After