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

Delegated PluginSuite initialization outside RootDatabaseBuilder #6838

Closed
wants to merge 1 commit into from

Conversation

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link
Member

@mkaput mkaput left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 20 files at r1, all commit messages.
Reviewable status: 1 of 20 files reviewed, 1 unresolved discussion (waiting on @integraledelebesgue and @orizi)


a discussion (no related file):
The number of changes here is tremendous to a level of alerting lights for me. Have you considered having a system of "use this plugin suite by default for crates if there are no per-crate ones"? This should make you avoid any added logic to tests

Copy link
Member Author

@integraledelebesgue integraledelebesgue left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 20 files reviewed, 1 unresolved discussion (waiting on @mkaput and @orizi)


a discussion (no related file):

Previously, mkaput (Marek Kaput) wrote…

The number of changes here is tremendous to a level of alerting lights for me. Have you considered having a system of "use this plugin suite by default for crates if there are no per-crate ones"? This should make you avoid any added logic to tests

Yeah, that would be useful in tests. This would require representing the default suite in the database and using it in the queries not being induced by Salsa's `inputs, all across the compiler. This sounds quite hacky to me. Also, it's relevant only in the context of testing so I haven't stuck with that idea.

Copy link
Member

@mkaput mkaput left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 20 files reviewed, 1 unresolved discussion (waiting on @integraledelebesgue and @orizi)


a discussion (no related file):

Previously, integraledelebesgue (Jan Smółka) wrote…

Yeah, that would be useful in tests. This would require representing the default suite in the database and using it in the queries not being induced by Salsa's `inputs, all across the compiler. This sounds quite hacky to me. Also, it's relevant only in the context of testing so I haven't stuck with that idea.

it is acceptable to have such things dedicated for tests if it benefits them A LOT, and this one will do. For example, this will allow you to keep shared db

@integraledelebesgue
Copy link
Member Author

Closing pull request: commit has gone away

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.

3 participants