Implement fmt::Display for TIR structures without requiring Env
#882
Conversation
kontheocharis
force-pushed
the
stores-display
branch
from
a7f3424
to
5cd6e91
Compare
compiler/hash-layout/src/compute.rs
Outdated
| let LayoutShape::Aggregate { fields: ref offsets, .. } = variant_layout.shape | ||
| else { | ||
| panic!("layout of enum variant is non-aggregate"); |
There was a problem hiding this comment.
this is pretty weird formatting, is this from rustfmt?
There was a problem hiding this comment.
Yes. I think that's the intended formatting.
compiler/hash-ast/src/tree.rs
Outdated
| @@ -53,7 +53,7 @@ impl AstVisitor for AstTreeGenerator { | |||
| "entry", | |||
| name.map(|t| TreeNode::branch("name", vec![t])) | |||
| .into_iter() | |||
| .chain(ty.map(|t| TreeNode::branch("type", vec![t])).into_iter()) | |||
| .chain(ty.map(|t| TreeNode::branch("type", vec![t]))) | |||
There was a problem hiding this comment.
I think that changes like this should be in their own PR in the future. If there is some PR unrelated fmt or clippy change, then I think the policy should be to make a separate PR for it and merge it before your main PR.
There was a problem hiding this comment.
I've made #883 to fix all the clippy and fmt issues, maybe we should merge this first and then rebase?
There was a problem hiding this comment.
I think that changes like this should be in their own PR in the future. If there is some PR unrelated fmt or clippy change, then I think the policy should be to make a separate PR for it and merge it before your main PR.
Yeah sounds reasonable but sometimes can be really annoying.. I think we don't have to be so dogmatic (not yet at least..) On second thought maybe it's worth it now that the compiler is getting so big.
|
In regards to clippy barfing, there seems to be some work being done to fix it (rust-lang/rust-clippy#11098). For now, we can disable the |
|
👋 coming from clippy. Interesting ICE you found. Just wanted to say that the linked PR doesn't fix this, but I've created an issue with a small repro: rust-lang/rust-clippy#11099 |
|
Also, this PR doesn't update |
|
I wonder if this issue is also relevant to the clippy crash rust-lang/rust-clippy#11064. Different line number but same rule. |
Will tackle this in this PR and then we can merge? |
|
Rebase and we're good to go |
This is so that Debug and Display can be properly implemented for them. This PR also unifies store and ID creation in the TIR under macros in hash_tir::environment::stores.
kontheocharis
force-pushed
the
stores-display
branch
from
923470a
to
24e2552
Compare
Pretty self explanatory. Closes #816. There is only one problem: when
ModDefs are printed, ones which correspond to source files, the formatter needs access to theSourceMapin order to resolve the path from theSourceId. However, the map is not static, and since theDisplayimplementations no longer have access toEnv, this is a problem. @feds01 thoughts on how to deal with this? One option is to make the SourceMap static, not sure if it is a good idea or not.