Conversation
`Deref` has an associated type `Target`. Normally it's a non-reference type, because the `deref` method provides the reference in its return type `&Self::Type`. There are several impls where `Target` is a reference type, which means the return type of `deref` is a double reference. This is necessary for `DiagCtxtHandle` and `TyCtxt` (due to how the lifetimes work) but unnecessary for the others. This commit changes the others to be more normal, which simplifies things in a few places.
|
Some changes occurred to the CTFE machinery Some changes occurred to the CTFE / Miri interpreter cc @rust-lang/miri Some changes occurred in match lowering cc @Nadrieril |
rustbot
added
S-waiting-on-review
|
@mejrs: welcome to the review rotation! :) |
| // straight-forward (`TagEncoding::Direct`) or with a niche (`TagEncoding::Niche`). | ||
| let (tag_scalar_layout, tag_encoding, tag_field) = match op.layout().variants { | ||
| let layout = op.layout(); | ||
| let (tag_scalar_layout, tag_encoding, tag_field) = match layout.variants { |
There was a problem hiding this comment.
Looks like here it doesn't actually simplify things, it forces us to manually spell out the temporary?
There was a problem hiding this comment.
The trade-off is that it's arguably worth having a vanilla Deref impl if it only causes one tiny piece of additional friction elsewhere.
There was a problem hiding this comment.
It doesn't look that bad to me. If I were to write this function, I'd probably hoist it regardless:
let layout = op.layout();
let ty = layout.ty;and then op.layout() -> layout throughout this function.
There was a problem hiding this comment.
I agree the cost is small, but I also don't see any downside to the current Deref impls. They are a bit non-standard, sure, but... so what?
| fn deref(&self) -> &&'a LayoutData<FieldIdx, VariantIdx> { | ||
| &self.0.0 | ||
| type Target = LayoutData<FieldIdx, VariantIdx>; | ||
| fn deref(&self) -> &Self::Target { |
There was a problem hiding this comment.
This does make the type strictly weaker -- previously one could copy out the inner, longer-lived shared reference to get a 'a reference; now that information is lost. I am not sure I agree with this being an improvement, it seems more like a step backwards to me?
| @@ -3074,10 +3074,10 @@ struct CallCtxt<'a, 'b, 'tcx> { | |||
| } | |||
|
|
|||
| impl<'a, 'b, 'tcx> Deref for CallCtxt<'a, 'b, 'tcx> { | |||
There was a problem hiding this comment.
These can be changed to placeholder lifetimes:
| impl<'a, 'b, 'tcx> Deref for CallCtxt<'a, 'b, 'tcx> { | |
| impl<'b, 'tcx> Deref for CallCtxt<'_, 'b, 'tcx> { |
Same for the other Deref impls you touched.
There was a problem hiding this comment.
In #155549 I eliminaetd one of the lifetimes anyway.
| // straight-forward (`TagEncoding::Direct`) or with a niche (`TagEncoding::Niche`). | ||
| let (tag_scalar_layout, tag_encoding, tag_field) = match op.layout().variants { | ||
| let layout = op.layout(); | ||
| let (tag_scalar_layout, tag_encoding, tag_field) = match layout.variants { |
There was a problem hiding this comment.
It doesn't look that bad to me. If I were to write this function, I'd probably hoist it regardless:
let layout = op.layout();
let ty = layout.ty;and then op.layout() -> layout throughout this function.
|
I don't have a strong opinion actually. I would approve these changes but I can see Ralfs perspective too. r? @RalfJung |
|
|
|
My objections are minor, but then the entire PR looks like a minor drive-by improvement (or is this working towards something bigger?). |
|
☔ The latest upstream changes (presumably #155552) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Ok, I have learned that returning a double reference from |
rustbot
removed
the
S-waiting-on-review
4 participants
Derefhas an associated typeTarget. Normally it's a non-reference type, because thederefmethod provides the reference in its return type&Self::Type.There are several impls where
Targetis a reference type, which means the return type ofderefis a double reference. This is necessary forDiagCtxtHandleandTyCtxt(due to how the lifetimes work) but unnecessary for the others. This commit changes the others to be more normal, which simplifies things in a few places.r? @mejrs