Constant::eq skips spans #85208
Constant::eq skips spans #85208
Conversation
|
If you change PartialEq, you also need to adjust Hash and HashStable to reflect these changes, otherwise hashmaps behave badly |
|
I think this is going to cause problems with incremental compilation. As @oli-obk said, this change would require ignoring the span in the |
|
yeah you both are right, I need to adjust that but I meant, wouldn't this cause incr. comp. issues regardless of the Hash and HashStable related changes?. Edit: ohh, sorry my english understanding skills are a bit poor :), I think that @Aaron1011 is exactly saying that indeed regardless of Hash and HashStable changes this will cause problems. Please confirm that to me because that was my assumption and I'm not sure if I understood correctly. In that case, would removing |
b6cb245 to
fa9e3d8
Compare
| @@ -683,14 +683,13 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { | |||
| self.stack_mut().push(frame); | |||
|
|
|||
| // Make sure all the constants required by this frame evaluate successfully (post-monomorphization check). | |||
| for const_ in &body.required_consts { | |||
| let span = const_.span; | |||
| for (span, const_) in &body.required_consts { | |||
There was a problem hiding this comment.
Oh right! we have required_consts. Perfect. I think we may be able to remove the span from Operand::Const after all
There was a problem hiding this comment.
Should we do this in a follow up PR? or do you want me to take some kind of action here?
| if let mir::Operand::Constant(box constant) = func { | ||
| if constant.span.lo() > span.lo() { | ||
| span = span.with_lo(constant.span.lo()); | ||
| if let mir::Operand::Constant(box (c_span, _)) = func { |
There was a problem hiding this comment.
not sure why this special cased to constants. we can do the same thing for other operands by looking up their local's span. But this is essentially what I was talking about for pulling the span even outside the Operand and using (Operand, Span) tuples where necessary
There was a problem hiding this comment.
I'm not sure I follow your comment, are you referring to something pre-existing? in that case, should we address it in a follow up PR? or are you asking me to change how I'm handling spans further?
There was a problem hiding this comment.
yea, I'm just musing on the pre-existing thing
| func: Operand::Constant(box Constant { | ||
| span: source_info.span, | ||
|
|
||
| func: Operand::Constant(box (source_info.span, Constant { |
There was a problem hiding this comment.
here we could pull the span into a field on TerminatorKind::Call
|
the current changes should always stay their own commit, this is really clean |
|
Triage, marking as experimental. |
|
closing this as it has bitrotted |
Opening this to start discussion and see what CI says about different approaches we can try for #77608.
For now I've just skipped span on
eqimplementation.r? @oli-obk
From what I've seen locally it passes all tests, which surprises me given the comments made on #77608. Although I'm not really sure how well this plays with incremental compilation, but my guess is that it doesn't play well.
Closes #77608