Skip to content
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

Go: Use getLocation instead of hasLocationInfo #18883

Merged
merged 3 commits into from
Feb 27, 2025

Conversation

owen-mc
Copy link
Contributor

@owen-mc owen-mc commented Feb 27, 2025

I'm sorry this isn't very easy to review. I did consider pulling it into separate commits: "Add getLocation", "Use getLocation instead of hasLocationInfo", "Make hasLocationInfo call getLocation and delete overrides", "Deprecate hasLocationInfo". If it really is impossible to review in its current state then I can do that.

As a test, I commented out all of the newly-deprecated predicates and ran all the tests, to check we aren't using them anywhere. They all passed.

@Copilot Copilot bot review requested due to automatic review settings February 27, 2025 12:39
@owen-mc owen-mc requested a review from a team as a code owner February 27, 2025 12:39

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Overview

This pull request deprecates the member predicate hasLocationInfo and instructs users to use getLocation() instead.

  • Introduced deprecation documentation for hasLocationInfo.

Reviewed Changes

File Description
go/ql/lib/change-notes/2025-02-27-haslocationinfo-deprecated.md New change note file that documents the deprecation of hasLocationInfo and suggests using getLocation()

Copilot reviewed 105 out of 105 changed files in this pull request and generated no comments.

Tip: Copilot code review supports C#, Go, Java, JavaScript, Markdown, Python, Ruby and TypeScript, with more languages coming soon. Learn more

Copy link
Contributor

@smowton smowton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Only reviewed the non-test changes on the assumption that test changes would show up in the results if they were materially wrong.

@owen-mc
Copy link
Contributor Author

owen-mc commented Feb 27, 2025

DCA summary:

  • About 10% reduction in cache size and tuple pool cache size for aws/aws-sdk-go (due to not caching hasLocationInfo tables?)
  • Analysis time for aws/aws-sdk-go went down by 5%. It could be noise, or it could be related to the cache sizes above.

@owen-mc owen-mc merged commit 76ad107 into github:main Feb 27, 2025
15 checks passed
@owen-mc owen-mc deleted the go/get-location branch February 27, 2025 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants