Begin splitting middle::ty #28274
Begin splitting middle::ty #28274
Conversation
|
Let the bikeshedding commence. |
|
☔ The latest upstream changes (presumably #27893) made this pull request unmergeable. Please resolve the merge conflicts. |
|
☔ The latest upstream changes (presumably #28300) made this pull request unmergeable. Please resolve the merge conflicts. |
|
I basically like this. I'm not sure about moving all the impls to a single module, but it might be nice. I've been wanting to split ty.rs into a directory for some time, for sure. @arielb1 this is just a reorganization, right? No behavioral changes? |
|
Right. No behavioral or (intentional) performance changes. |
|
I didn't move all impls, just |
| /// value's 'lt exist | ||
| /// | ||
| /// NonZero is used rather than Unique because Unique isn't Copy. | ||
| pub struct TyIVar<'tcx, 'lt: 'tcx>(ivar::Ivar<NonZero<*const TyS<'static>>>, |
There was a problem hiding this comment.
I'm not a huge fan of having a redundant prefix on the name, but I guess you are doing it to disambiguate with Ivar.
There was a problem hiding this comment.
It is an Ivar<Ty<'tcx>>, not inline because of variance being annoying.
|
I like this as well 👍, but after 1214, MIR, and HIR this is going to be another "fun" rebase on my branch. |
So I guess the interesting question is where these impls should go in general. I've sometimes regretted the current style -- I tend to think the impls should probably go with the types (though it's natural when first defining the traits to stick them with the trait). This is particularly true since in the cross-crate case they must go with the types. Note that "impls go with the types" might be loosely defined -- e.g., all types defined in the |
|
r+ from me, in any case. This looks like progress to me. I second the... joyful thoughts of rebases to come. |
|
90% of the impls are for ty and rustc dependencies (std, syntax, etc.), and therefore would go in |
|
☔ The latest upstream changes (presumably #28392) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@arielb1 I think I would prefer that, yes. The |
|
But then I see you already did it. |
|
@bors r+ |
|
📌 Commit 5a95acb has been approved by |
That file got way too big for its own good. It could be split more - this is just a start. r? @nikomatsakis
|
Are |
That file got way too big for its own good. It could be split more - this is just a start.
r? @nikomatsakis