Skip to content

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 AI 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
Copy link
Contributor

Copilot AI left a comment

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