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

Crate structure cleanup #456

Closed
3 tasks
jackh726 opened this issue May 14, 2020 · 9 comments · Fixed by #459
Closed
3 tasks

Crate structure cleanup #456

jackh726 opened this issue May 14, 2020 · 9 comments · Fixed by #459
Assignees

Comments

@jackh726
Copy link
Member

jackh726 commented May 14, 2020

There are a few different crate structure cleanups we want to do.

  • Move recursive solver out of chalk-solve
    • A lot of the functionality here overlaps with chalk-engine, so it would be nice try to set up the crate structure in a way that we only need to compile one or the other.
  • Merge chalk-rust-ir into chalk-solve
    • This is a pretty small crate and there's not really a benefit to keeping it split.
  • Move chalk-macros into chalk-engine
    • chalk-macros is also pretty small. This might be a little different, depending on the how the first bullet point goes. But basically, this doesn't need to be all by itself.

This issue has been assigned to @Mcat12 via this comment.

@AzureMarker
Copy link
Member

I can take this, though I have a question about "Merge chalk-rust-ir into chalk-solve". Will it need to be moved out again once a common type library is shared with rustc? Keeping it out of chalk-solve would therefore be closer to the target state.

@rustbot claim

@rustbot rustbot self-assigned this May 16, 2020
@AzureMarker
Copy link
Member

Is the SLG solver wholly contained within chalk-engine, or is it also in chalk-solve? It may make sense to have a structure where chalk-engine provides the shared types between the SLG and recursive solvers. The solvers would be in their own chalk-slg-solver/chalk-recursive-solve crates and depend on chalk-engine. Then chalk-solve would depend on all three crates. Does this make sense?

@jackh726
Copy link
Member Author

But to answer your questions @Mcat12, the types that we need to share are all in chalk-ir.

As far as SLG, the core logic is all inchalk-engine, but the Slg context and aggregate logic is in chalk-solve. I was also think that there would be a shared crate that both slg and recursive solvers depend on. And solve depends on both. But I don't really know if about the recursive solver to say how much is actually shared.

@zaharidichev
Copy link
Contributor

Yeah I wanted to but life is first come first served :) So take a stab @Mcat12! @jackh726 Anything else that I can dig into (something hard)?

@AzureMarker
Copy link
Member

AzureMarker commented May 16, 2020

That's fine, I can unassign myself assuming he's still interested.

Do you know of other issues that might help me learn the codebase? I'm at an "intermediate" level with the codebase, but I found some issues that may be doable. Perhaps you would have a better idea which would be most suitable? #313 #429

@rustbot release-assignment

Edit: Doh! 10 seconds too late

@rustbot rustbot removed their assignment May 16, 2020
@jackh726
Copy link
Member Author

Let me open a topic on Zulip for a few things people can work on!

@AzureMarker
Copy link
Member

Thanks for the Zulip thread @jackh726 (link)! I'll re-claim this issue; it looks like @zaharidichev will be working on #313.

@rustbot claim

@rustbot rustbot self-assigned this May 16, 2020
@AzureMarker
Copy link
Member

AzureMarker commented May 17, 2020

Based on a Zulip discussion, we'll keep the recursive solver in chalk-solve for now but still have the "chalk-engine-base" crate which holds the few shared solver types (replacing chalk-macros). There will be crate features to disable the SLG/recursive solvers, to avoid unnecessary compilation.

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 a pull request may close this issue.

4 participants