-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Move structures from ty/mod.rs
into their own modules
#111208
The head ref may contain hidden characters: "\u{1F338}cutify_ty\u{1F338}"
Conversation
This comment has been minimized.
This comment has been minimized.
compiler/rustc_hir_typeck/src/fn_ctxt/adjust_fulfillment_errors.rs
Outdated
Show resolved
Hide resolved
This is a lot of submodules :/ Is there any way of grouping these up into something a bit more coherent than "one module per type"? |
@compiler-errors there probably is, but my point was that with small submodules it's easier to group them, so this is the first step. |
I understand the approach, but I don't think there should be, for example, a separate submodule for all the different predicate internals (clause, typredicate, projection pred, etc.). Same with ty, term, paramterm which are conceptually grouped. Same with ADT internals (fields, variants, etc.) all should probably live together in one module** (typo). Impl header stuff and coherence iternals (ImplHeader, ImplOverlapKind, ImplSubject) should all probably live together in one place. Bound const should live with const related stuff. Also this is a lot of commits for just moving stuff around... |
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.
Sorry, here's a slightly messier but more concise summary of all the groupings I think need to be done here.
As I said before, I think the philosophy of breaking up rustc_middle::ty into more coherent groupings is a good idea, but this needs some refactoring hehe.
compiler/rustc_middle/src/ty/ty_.rs
Outdated
@@ -0,0 +1,9 @@ | |||
use crate::ty::{Interned, TyKind, WithCachedTypeInfo}; |
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.
there shouldn't be both an sty and ty i think lol
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.
Yeah, I'll work on sty
after this PR 😅
use crate::ty::TyCtxt; | ||
|
||
#[derive(Clone, Debug, PartialEq, Eq, Copy, Hash, Encodable, Decodable, HashStable)] | ||
pub enum Visibility<Id = LocalDefId> { |
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.
idk... maybe this should live with adt stuff? even tho vis is used for more than that... i guess it's fine to live alone 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.
Probably a submodule of rustc_middle::middle
.
This comment has been minimized.
This comment has been minimized.
☔ The latest upstream changes (presumably #111342) made this pull request unmergeable. Please resolve the merge conflicts. |
…tlivesPredicate, RegionOutlivesPredicate, TypeOutlivesPredicate}` to their own little module (cute)
…n little module (cute)
…InherentImpls, CrateVariancesMap, Destructor, DestructuredConst, ImplHeader, ImplOverlapKind, ImplSubject, InferVarInfo, OpaqueTypeKey, VariantDiscr, VariantFlags}` to their own little modules (cute)
... and add `MainDefinition` to it
This comment was marked as off-topic.
This comment was marked as off-topic.
Github borked my PR again, ugh. Sorry for the pings, everyone. Closing as this seems to be beyond repair similarly to #110795. Will open a new version. |
Nevermind, this time close+reopen actually helped. |
☔ The latest upstream changes (presumably #111358) made this pull request unmergeable. Please resolve the merge conflicts. |
Github is being weird again, so I've reopened the PR here: #111503 |
The main goal here is to allow easier maintenance of each individual structure, but this may help us make better module structure as well.
r? @oli-obk (you are usually ok with my big dumb refactoring PRs (also I'm sorry))