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

Java: Clean up some instances of getQualifiedName. #16478

Merged
merged 1 commit into from
May 14, 2024

Conversation

aschackmull
Copy link
Contributor

Just cleaning up some tech debt.

@aschackmull aschackmull added the no-change-note-required This PR does not need a change note label May 13, 2024
@aschackmull aschackmull requested a review from a team as a code owner May 13, 2024 11:09
@github-actions github-actions bot added the Java label May 13, 2024
Copy link
Contributor

@atorralba atorralba left a comment

Choose a reason for hiding this comment

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

LGTM, with one comment.

Comment on lines -35 to +36
cached
pragma[nomagic]
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to be sure: what are the implications of this in terms of performance? Isn't this a bit out of place in an otherwise harmless-looking PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Firstly, cached implies nomagic, so when removing a cached annotation, the smallest possible change is to then also add nomagic, hence the pragma addition. And caching doesn't really make sense in a .ql file - it's not like there's some other query that can reference the predicate, so this was just a drive-by clean-up and should be harmless.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I didn't realize it was a query file. TIL cached implies nomagic (it's somewhat obvious in hindsight, actually). Thanks for the explanation!

@aschackmull aschackmull merged commit ed0717a into github:main May 14, 2024
16 checks passed
@aschackmull aschackmull deleted the java/hasqualifiedname branch May 14, 2024 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Java no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants