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

Multiple trie #16098

Merged
merged 11 commits into from
Oct 13, 2023
Merged

Multiple trie #16098

merged 11 commits into from
Oct 13, 2023

Conversation

nojaf
Copy link
Contributor

@nojaf nojaf commented Oct 9, 2023

In #16085, another "ghost dependency" case was solved.
It was, however, becoming a little less clear why the fix was necessary.
This PR is an attempt to address this.

So, recap time, what happens against in mkGraph?
We currently construct a single Trie with nodes we encountered in the files.
Once we have that, we process all identifiers in that project and do look-ups in Trie to detect any dependencies. The downside of this approach is that look-ups can happen in the Trie and hits can be found for files that came after the current file index.

We currently filtered those out while we were querying but it might be more easy to reason about this when we build up the Trie incrementally for each file. In short, we use a Trie for each file that only contains the nodes from files that came before it. This way, we never need to filter out anything based on the index.

@nojaf nojaf force-pushed the multiple-trie branch 3 times, most recently from bace533 to bff1748 Compare October 10, 2023 12:50
@nojaf nojaf marked this pull request as ready for review October 12, 2023 16:18
@nojaf nojaf requested a review from a team as a code owner October 12, 2023 16:18
@T-Gro T-Gro enabled auto-merge (squash) October 13, 2023 08:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants