Skip to content

Implement cycle detection in hls-graph #2756

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 11 commits into from
Mar 8, 2022
Merged

Implement cycle detection in hls-graph #2756

merged 11 commits into from
Mar 8, 2022

Conversation

pepeiborra
Copy link
Collaborator

@pepeiborra pepeiborra commented Mar 6, 2022

Almost completely lifted from Shake.

I also started a barebones test suite for hls-graph and removed some legacy option placeholders (shakeThreads and shakeFiles)

@pepeiborra pepeiborra requested a review from jneira as a code owner March 6, 2022 15:41
@michaelpj
Copy link
Collaborator

Time to go back to Shake? :trollface:

@pepeiborra
Copy link
Collaborator Author

pepeiborra commented Mar 7, 2022

Since I am the sole maintainer of hls-graph, we need someone else to review my PRs. Are you happy to fill in that role @michaelpj @wz1000 ?

@@ -165,7 +170,7 @@ compute db@Database{..} key mode result = do
deps | not(null deps)
&& runChanged /= ChangedNothing
-> do
void $ forkIO $
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed this forkIO because it made testing reverse deps impossible, and I can't justify the need for it anyway

@michaelpj
Copy link
Collaborator

I can give it a look, but if you'd like a substantive review I'll need to devote some time to understanding what is actually going on.

@wz1000
Copy link
Collaborator

wz1000 commented Mar 8, 2022

Looks fairly straightforward. Allocations in the Cabal benchmark seem to be a bit higher, but I guess that is to be expected.

@pepeiborra pepeiborra merged commit 6a8dc22 into master Mar 8, 2022
July541 pushed a commit to July541/haskell-language-server that referenced this pull request Mar 30, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants