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.
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
Client store generalization #19420
Client store generalization #19420
Changes from all commits
f1b9673
2b4b6bd
1bdf521
b026776
52f7cc1
152c499
1aada91
61953be
c2e982e
0a2062c
bf210ba
c085904
33baa6e
1ba330c
04bf1db
cce3360
0f0dbef
bdc93f1
9143225
c9378ec
7a2f18a
97f9e9a
ad1f737
06af01f
d94d390
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
nit: I'm not sure about the performance differences, but you could use
DeduplicateAny()
in the implementation here: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.
Deduplicate[T comparable]
has better performance since it can use amap[T]bool
for O(1) existence checks, so I think it's better not to generalize all uses to the currentDeduplicateAny
with O(n) existence checks. Let me know if you disagree since the performance is pretty negligible to be honest.I was considering changing
DeduplicateAny
tofunc[T any](in []T, hashFunc func(T) string) []T
soDeduplicate
could simply call that, but I don't think it's worth the extra complexity right now.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.
nit: I'd be nice to also unit test the
Deduplicate()
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.
These tests already exist above here, no change was necessary for the generification.