Move pointee_info_at from rustc_codegen_llvm to rustc_target.#60237
Move pointee_info_at from rustc_codegen_llvm to rustc_target.#60237bors merged 16 commits intorust-lang:masterfrom
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
|
r? @eddyb |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Creating a new PR as I do not have push rights to @wildarch 's repo |
|
There are still some points unresolved from the last PR. I think something about the associated types and creating a new trait?On 24 Apr 2019 17:49, Saleem Jaffer <notifications@github.com> wrote:Resolves #56166.
This is a continuation of #60237.
@oli-obk Should I close the older PR?
You can view, comment on, or merge this pull request online at:
#60237
Commit Summary
Move pointee_info_at to TyLayoutMethods.Fix typo in src/librustc/ty/layout.rsReturn instead of collecting to mut result.Remove old pointee_info_at body.Add param_env parameter to pointee_info_at.Make line fit within 100 character limit.impl `pointee_info_at` in TyLayout.resolving conflicts
File Changes
M
src/librustc/ty/layout.rs
(136)
M
src/librustc_codegen_llvm/abi.rs
(8)
M
src/librustc_codegen_llvm/context.rs
(3)
M
src/librustc_codegen_llvm/type_of.rs
(134)
M
src/librustc_target/abi/mod.rs
(34)
Patch Links:
https://github.com/rust-lang/rust/pull/60237.patchhttps://github.com/rust-lang/rust/pull/60237.diff
—You are receiving this because you were mentioned.Reply to this email directly, view it on GitHub, or mute the thread.
|
|
@oli-obk Yeah, I have not yet fixed any issues. I just rebased on top of master. Will start fixing them. |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
76fa80d to
f2935a3
Compare
|
@oli-obk This is the fallout: |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
src/librustc/ty/layout.rs
Outdated
There was a problem hiding this comment.
So... apparently doing it this way is blocked on lazy normalization: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=e6956340922d4df0087baa36b02b1ce3
Let's go with your original suggestion and add a from(r: Result<T, Self::item>) method to MaybeResult
5298959 to
738f9c3
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
738f9c3 to
bafa063
Compare
src/librustc/ty/layout.rs
Outdated
There was a problem hiding this comment.
@oli-obk I get this error here:
error[E0308]: mismatched types==============================> ] 118/137: rustc
--> src/librustc/ty/layout.rs:1692:32
|
1656 | fn field(this: TyLayout<'tcx>, cx: &C, i: usize) -> C::TyLayout {
| ----------- expected `<C as rustc_target::abi::LayoutOf>::TyLayout` because of return type
...
1692 | return ptr_layout;
| ^^^^^^^^^^ expected associated type, found struct `rustc_target::abi::TyLayout`
|
= note: expected type `<C as rustc_target::abi::LayoutOf>::TyLayout`
found type `rustc_target::abi::TyLayout<'_, &ty::TyS<'_>>`
Why is to_result changing the type here?
There was a problem hiding this comment.
The error is sadly not telling you what the actual issue is. What you want here is to convert the result back to a MaybeResult after mapping it
return MaybeResult::from(cx.layout_of(ptr_ty).to_result().map(|mut ptr_layout| {
ptr_layout.ty = this.ty;
ptr_layout
}));There was a problem hiding this comment.
@oli-obk Should this be raised as an issue? :o
There was a problem hiding this comment.
I think we used to have some more aggressive suggestions around that. If you can create a small example, then a diagnostics issue would be great, yes.
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
src/librustc/ty/layout.rs
Outdated
There was a problem hiding this comment.
The error is sadly not telling you what the actual issue is. What you want here is to convert the result back to a MaybeResult after mapping it
return MaybeResult::from(cx.layout_of(ptr_ty).to_result().map(|mut ptr_layout| {
ptr_layout.ty = this.ty;
ptr_layout
}));bafa063 to
e983574
Compare
src/librustc/ty/layout.rs
Outdated
There was a problem hiding this comment.
@eddyb what do you think about making is_freeze take TyCtxtAt and make HasTyCtxt::tcx return a TyCtxtAt?
There was a problem hiding this comment.
I don't think we should bother, for now.
If you want, you can just make it not a method of Ty.
src/librustc_target/abi/mod.rs
Outdated
There was a problem hiding this comment.
You can remove this, it's always part of cx, because all implementors need it in order to implement layout_of anyway!
11d3504 to
94a4892
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
src/librustc/ty/layout.rs
Outdated
| ptr_layout | ||
| }); | ||
| return MaybeResult::from( | ||
| cx.layout_of(ptr_ty).to_result().map(|mut ptr_layout| { |
There was a problem hiding this comment.
Even with this much indentation, rustfmt will keep return MaybeResult::from(cx.layout_of(ptr_ty).to_result().map( on one line and just have the closure expression indented by itself, e.g.:
return MaybeResult::from(cx.layout_of(ptr_ty).to_result().map(
|mut ptr_layout| {
ptr_layout.ty = this.ty;
ptr_layout
},
));There was a problem hiding this comment.
@eddyb actually rustfmt does this
return MaybeResult::from(cx.layout_of(ptr_ty).to_result().map(|mut ptr_layout| {
ptr_layout.ty = this.ty;
ptr_layout
}));There was a problem hiding this comment.
Only if you have less indentation than you actually do here.
src/librustc_target/abi/mod.rs
Outdated
| where Ty: TyLayoutMethods<'a, C>, C: LayoutOf<Ty = Ty> { | ||
| Ty::pointee_info_at(self, cx, offset, param_env) | ||
| } | ||
| pub fn is_freeze<C>(self, cx: &C, param_env: Ty::ParamEnv) -> bool |
There was a problem hiding this comment.
Huh, was this actually not needed? Only pointee_info_at?
Or was my original plan to implement pointee_info_at in rustc_target instead?
I guess I had forgotten about the special-cases in it? A bit surprising, still, though.
There was a problem hiding this comment.
I was referring to the param_env argument, not is_freeze.
But I think in your first link, I did forget that you can't implement pointee_info_at in rustc_target anyway.
|
@bors r+ |
|
📌 Commit 968eb7f has been approved by |
|
☀️ Test successful - checks-travis, status-appveyor |
Makes progress towards #56166.
This is a continuation of #57150.
@oli-obk Should I close the older PR?