chore: Simplify location uniquing #63263
Merged
Conversation
This file contains 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
varungandhi-src
commented
Jun 14, 2024
| @@ -354,22 +354,12 @@ func (s *store) extractLocationsFromPosition( | |||
| } | |||
| } | |||
|
|
|||
| return deduplicateLocations(locations), collections.DeduplicateBy(symbols, func(s string) string { return s }), nil | |||
There was a problem hiding this comment.
Before this, there is a call
locations = append(locations, convertSCIPRangesToLocations(ranges, locationKey.UploadID, locationKey.Path)...)
So all locations have the same UploadID and Path.
kritzcreek
approved these changes
Jun 14, 2024
| @@ -354,22 +354,12 @@ func (s *store) extractLocationsFromPosition( | |||
| } | |||
| } | |||
|
|
|||
| return deduplicateLocations(locations), collections.DeduplicateBy(symbols, func(s string) string { return s }), nil | |||
| // We only need to unique by the range, since all location objects share the same path and uploadID. | |||
There was a problem hiding this comment.
Suggested change
| // We only need to unique by the range, since all location objects share the same path and uploadID. | |
| // We only need to deduplicate by range, since all location objects share the same path and uploadID. |
| seen.Add(x) | ||
| filtered = append(filtered, x) | ||
| } | ||
| return filtered |
There was a problem hiding this comment.
Should probably call slices.Clip on the result
There was a problem hiding this comment.
For defensiveness in the case of aliasing? Clipping the result will trigger a copy on future appends.
There was a problem hiding this comment.
Yeah that was my thinking. Honestly slices terrify me...
There was a problem hiding this comment.
If you want to be actually defensive in the case of aliasing, you'd have to construct a copy, and we don't want to do that here. I don't think it's worth taking half-measures like this.
kritzcreek
requested changes
Jun 14, 2024
kritzcreek
approved these changes
Jun 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.
The code earlier constructing the locations attaches the same
uploadID and path to all of them, so trying to use those while
uniquing is wasteful. Let's stop doing that.
Test plan
Covered by existing tests
Changelog