-
Notifications
You must be signed in to change notification settings - Fork 182
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
Migrate Vec<ParameterKinds<()>>
and Vec<ParameterKinds<UniverseIndex>>
to interned
#386
Conversation
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.
OK, I'll want to look more, but first common is that I would prefer to avoid a setup like Canonical<I, T>
and instead require that T: HasInterner
-- do you know if there's a good reason we can't do that?
@nikomatsakis Ok, i've rebased and reverted those changes. Ready for review again. |
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.
Based on a quick scroll, this is looking good, modulo my comment below, which I suspect is more concerned with follow-up work
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.
Next suggestion =) I'm also debating about renaming ParameterKindsWithUniverseIndex
, as I noted on Zulip
Comments addressed. Ready for review again. |
fn into_binders_and_value(self) -> (Self::Binders, Self::Value) { | ||
(self.0.binders.iter(self.1).cloned(), self.0.skip_binders()) | ||
fn into_binders_and_value(self, interner: &'a I) -> (Self::Binders, Self::Value) { | ||
(self.binders.iter(interner).cloned(), self.skip_binders()) |
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.
Oh, I see, it's because we actually return an interior here -- well, this seems ok.
@crlf0710 can you rebase this PR? Then I'm inclined to merge! |
@nikomatsakis Sure, done! |
@crlf0710 uh oh, need one more rebase :) |
@nikomatsakis Done again :) |
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.
Found one inaccurate comment. I may just push up a fix for that myself.
// different from `^1.0` in terms of what variable is being | ||
// referenced. | ||
write!(fmt, "<")?; | ||
for (index, binder) in self.parameter_kinds.iter(self.interner).enumerate() { |
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.
Wait, this code doesn't seem like it's in the right place. ParameterKinds are just a list of, well, parameter kinds. They are not themselves a binder... Ah, well, I guess every time they are used, it's as part of a binder... ok, seems reasonable. 👍
closes #369 . But i guess some adjustment are needed.