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

Remove global variable from dir_status #1573

Merged
merged 1 commit into from
Dec 3, 2018

Conversation

rgrinberg
Copy link
Member

Now we store the dir_status database per super context. Similarly, we could remove the global cache in Dir_contents as well if I manage to make #1566 work.

@rgrinberg rgrinberg requested a review from a user November 25, 2018 20:33
@ghost
Copy link

ghost commented Nov 26, 2018

FTR, I'm counting of #1489 to get rid of all these global variables. The idea is that getting the directory status will just become a memoized function and Dune will automatically track what computing the directory status requires. So we shouldn't bother getting rid of these global variables for now.

@rgrinberg
Copy link
Member Author

The idea is that getting the directory status will just become a memoized function and Dune will automatically track what computing the directory status requires

Oh ok, I thought that getting rid of the global variables would help transitioning it to #1489. Since it now becomes what are the data dependencies of Dir_status.

@ghost
Copy link

ghost commented Nov 26, 2018

It's possible, but I think the situation will be clearer once #1489 is merged and we can start refactoring the code.

@rgrinberg rgrinberg reopened this Nov 30, 2018
@rgrinberg
Copy link
Member Author

I'm re-opening because inverting the dep here is required for other changes. So unless this PR makes it harder to migrate to #1489, I'd like to see it merged, even if this code will be removed in the near future.

Store the state in a DB type

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
@rgrinberg rgrinberg force-pushed the remove-sctx-dir-status-contents branch from 22a1904 to 5cbe505 Compare December 3, 2018 21:18
@rgrinberg rgrinberg merged commit 0d55e2d into ocaml:master Dec 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant