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

Ruby: put AST node locations in a single table #7875

Merged
merged 6 commits into from
Feb 7, 2022

Conversation

nickrolfe
Copy link
Contributor

@nickrolfe nickrolfe commented Feb 7, 2022

This should make getLocation() on generated nodes faster.

I generated the upgrade/downgrade scripts with some quick hacks to the generator program, and only hand-edited the results for formatting. That means reviewers should feel confident that, for the upgrades/downgrades, they only need to check:

  • {ruby,erb}_ast_node_parent <-> {ruby,erb}_ast_node_info upgrades/downgrades.
  • {ruby,erb}_tokeninfo modifications
  • and just a small sample of the *_def table modifications.

I tested them by:

  1. Running all the qltests with --search-path=main-extractor-pack:ql so that they would use an extractor from main and upgrade the dbs on the fly.
  2. Building a large Ruby database with an extractor from main, upgrading it, then downgrading it, and checking that all the relations in the original and downgraded databases were byte-for-byte identical.

@github-actions github-actions bot added the Ruby label Feb 7, 2022
@nickrolfe nickrolfe marked this pull request as ready for review February 7, 2022 13:21
@nickrolfe nickrolfe requested a review from a team as a code owner February 7, 2022 13:21
Copy link
Contributor

@aibaars aibaars left a comment

Choose a reason for hiding this comment

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

LGTM

@nickrolfe
Copy link
Contributor Author

Let's at least wait for DCA to finish before merging.

@nickrolfe
Copy link
Contributor Author

The DCA results look good, and show a nice speedup on analysis time.

@nickrolfe nickrolfe merged commit 9217d0e into main Feb 7, 2022
@nickrolfe nickrolfe deleted the nickrolfe/locations_column branch February 7, 2022 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants