-
Notifications
You must be signed in to change notification settings - Fork 556
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
Introduced DynEq + DynHash
bounds for MacroPlugin
, InlineMacroExprPlugin
and AnalyzerPlugin
traits
#6839
Conversation
…prPlugin` and `AnalyzerPlugin` traits commit-id:57b41a03
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 31 of 31 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @integraledelebesgue)
-- commits
line 2 at r1:
please elaborate as to why you need this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @orizi)
Previously, orizi wrote…
please elaborate as to why you need this.
I don't think we want to go this route. Have you considered just hashing and comparing pointers that are underlying to Arc<MacroPlugin>
at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Draggu)
Previously, mkaput (Marek Kaput) wrote…
I don't think we want to go this route. Have you considered just hashing and comparing pointers that are underlying to
Arc<MacroPlugin>
at all?
That was my first idea :) Such an approach is fine for plugins being unit structs, but is it safe to generalize it for non-unit plugins, especially the Scarb plugin? cc: @Draggu
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 31 of 31 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Draggu)
Previously, integraledelebesgue (Jan Smółka) wrote…
That was my first idea :) Such an approach is fine for plugins being unit structs, but is it safe to generalize it for non-unit plugins, especially the Scarb plugin? cc: @Draggu
wdym by Scarb plugin?
no plugin known to me has any state (and having one is pretty hardcore idea) so we can afford having duplicate instances of a plugin in worst case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Draggu)
Previously, mkaput (Marek Kaput) wrote…
wdym by Scarb plugin?
no plugin known to me has any state (and having one is pretty hardcore idea) so we can afford having duplicate instances of a plugin in worst case
I'm talking about plugins like, for example, ProcMacroHostPlugin
from Scarb. It's not a unit struct. At the same time, it becomes a part of a plugin suite loaded into a RootDatabase
during Scarb compilation here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Draggu)
Previously, integraledelebesgue (Jan Smółka) wrote…
I'm talking about plugins like, for example,
ProcMacroHostPlugin
from Scarb. It's not a unit struct. At the same time, it becomes a part of a plugin suite loaded into aRootDatabase
during Scarb compilation here.
I'm not saying we cannot compare pointers here, just wonder if that's a good approach for all possible forms of plugins.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Draggu)
Previously, integraledelebesgue (Jan Smółka) wrote…
I'm not saying we cannot compare pointers here, just wonder if that's a good approach for all possible forms of plugins.
discussed offline -> let's go with pointer comparisons
Closing pull request: commit has gone away |
Stack:
semantic
testing utilities #6841DefsGroup
andSemanticGroup
#6840DynEq + DynHash
bounds forMacroPlugin
,InlineMacroExprPlugin
andAnalyzerPlugin
traits #6839 ⬅PluginSuite
initialization outsideRootDatabaseBuilder
#6838