Skip to content

C++: Merge the location tables #17581

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

jketema
Copy link
Contributor

@jketema jketema commented Sep 25, 2024

See internal PR for details.

Pull Request checklist

All query authors

Internal query authors only

  • Autofixes generated based on these changes are valid, only needed if this PR makes significant changes to .ql, .qll, or .qhelp files. See the documentation (internal access required).
  • Changes are validated at scale (internal access required).
  • Adding a new query? Consider also adding the query to autofix.

@github-actions github-actions bot added the C++ label Sep 25, 2024
@jketema jketema force-pushed the loc-table-merge branch 10 times, most recently from 9299482 to c05417a Compare October 1, 2024 11:00
@jketema jketema added the depends on internal PR This PR should only be merged in sync with an internal Semmle PR label Oct 1, 2024
@jketema jketema force-pushed the loc-table-merge branch 2 times, most recently from f6d5deb to a8ce943 Compare November 18, 2024 10:51
@jketema jketema force-pushed the loc-table-merge branch 2 times, most recently from ea8b00b to 52af6d5 Compare June 27, 2025 12:24
@jketema jketema marked this pull request as ready for review June 27, 2025 13:01
@Copilot Copilot AI review requested due to automatic review settings June 27, 2025 13:01
@jketema jketema requested review from a team as code owners June 27, 2025 13:01
Copy link
Contributor

@Copilot 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.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@@ -218,10 +218,10 @@ extractor_version(
string frontend_version: string ref
)

@location = @location_stmt | @location_expr | @location_default ;
@location = @location_default ;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can location_default be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need it, because xmllocations table uses it, and the xml tables are effectively shared across the dbschemes of different languages. There's one extractor for XML files, and it expects that part of the dbscheme to be identical for all languages. Renaming @location_default to @location wouldn't really affect things functionally, but will be slightly confusing if you work across languages.

An alternative would be to drop @location completely and call everything @location_default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C++ depends on internal PR This PR should only be merged in sync with an internal Semmle PR documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants