-
Notifications
You must be signed in to change notification settings - Fork 13k
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 pointee_info_at from rustc_codegen_llvm to rustc_target. #60237
Move pointee_info_at from rustc_codegen_llvm to rustc_target. #60237
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 <[email protected]> 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
@@ -1490,27 +1492,40 @@ impl<'gcx, 'tcx, T: HasTyCtxt<'gcx>> HasTyCtxt<'gcx> for LayoutCx<'tcx, T> { | |||
} | |||
} | |||
|
|||
pub trait MaybeResult<T> { | |||
pub trait MaybeResult<T, E>: From<Result<T, E>> { |
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.
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
@@ -1694,10 +1686,11 @@ impl<'a, 'tcx, C> TyLayoutMethods<'tcx, C> for Ty<'tcx> | |||
} else { | |||
tcx.mk_mut_ref(tcx.types.re_static, nil) | |||
}; | |||
return cx.layout_of(ptr_ty).map_same(|mut ptr_layout| { | |||
let ptr_layout: C::TyLayout = cx.layout_of(ptr_ty); |
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.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@oli-obk Should this be raised as an issue? :o
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.
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
@@ -1694,10 +1686,11 @@ impl<'a, 'tcx, C> TyLayoutMethods<'tcx, C> for Ty<'tcx> | |||
} else { | |||
tcx.mk_mut_ref(tcx.types.re_static, nil) | |||
}; | |||
return cx.layout_of(ptr_ty).map_same(|mut ptr_layout| { | |||
let ptr_layout: C::TyLayout = cx.layout_of(ptr_ty); |
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.
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
cx: &C, | ||
param_env: Self::ParamEnv, | ||
)-> bool { | ||
this.ty.is_freeze(cx.tcx(), param_env, DUMMY_SP) |
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.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
fn is_freeze( | ||
this: TyLayout<'a, Self>, | ||
cx: &C, | ||
param_env: Self::ParamEnv, |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only if you have less indentation than you actually do here.
src/librustc_target/abi/mod.rs
Outdated
@@ -969,10 +964,6 @@ impl<'a, Ty> TyLayout<'a, Ty> { | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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?