Expand VisitMachineValues to cover more pointers in the interpreter #2566
Conversation
saethlin
force-pushed
the
gc-cleanup
branch
from
deca8e9 to
19bd4ac
Compare
saethlin
added
S-waiting-on-author
| @@ -3,30 +3,120 @@ use rustc_data_structures::fx::FxHashSet; | |||
| use crate::*; | |||
|
|
|||
| pub trait VisitMachineValues { | |||
| fn visit_machine_values(&self, visit: &mut impl FnMut(&Operand<Provenance>)); | |||
| fn visit_machine_values(&self, visit: &mut ProvenanceVisitor); | |||
| } | |||
There was a problem hiding this comment.
Hm okay this is less general than what I started out with, but I guess for now visiting all provenance is sufficient.
But then it should probably be called VisitProvenance.
And why did you change the visit argument?
There was a problem hiding this comment.
We need to visit a number of different things. It's possible to contort them all into an Operand, but that adds a lot of repetitive glue code at the call sites. If each use of the visitor can take a different type, we can do the dispatch inside this module, instead of spread across the codebase. So we need to shift the generic parameter to each call to the visitor, which means we need a new type with the generic parameter on the method, not FnMut.
There was a problem hiding this comment.
But it seems you solved this by completely re-structuring the visitor to (a) to longer be lazy (it now hard-codes the use of a hash map -- ProvenanceVisitor is a somewhat misleading name for a newtyped HashMap) and (b) hard-code that only provenance matters.
If we want to hard-code that only provenance matters (which I am fine with for now), we should have
pub trait VisitProvenance {
fn visit_provenance(&self, visit: &mut impl FnMut(Provenance));
}Then you can impl VisitProvenance for {Operand,Immediate,Pointer} and you can now do x.visit_provenance(visit) for x ∈ {Operand,Immediate,Pointer}.
If we don't want to hard-code hat only provenance matters we need something else, but your approach does not achieve that.
There was a problem hiding this comment.
That said, some places only have SbTag, not a full Provenance... not sure what the best way is to handle them. We could make the visitor focus only on SbTag I guess; then it should be called VisitProvenanceTags or so.
There was a problem hiding this comment.
I pushed to this branch a version of these traits (or rather, just a single trait) demonstrating what I meant.
saethlin
added
S-waiting-on-author
RalfJung
force-pushed
the
gc-cleanup
branch
3 times, most recently
from
b30d3a9 to
1776178
Compare
|
I went over all the visitors and made sure the discarded parts with But yeah some kind of macro would be great.^^ Even a regular macro where one lists all the fields and that does the pattern expansion and calling the visitor. rustc used to have something like that but then later it got turned into a proper proc macro. |
|
Ah, CI already merges with master. We can't rebase due to the subtree sync situation. |
|
@bors r=oli-obk |
Expand VisitMachineValues to cover more pointers in the interpreter Follow-on to #2559 This is making me want to write a proc macro 🤔 r? `@RalfJung`
|
💔 Test failed - checks-actions |
|
@bors r=oli-obk |
|
☀️ Test successful - checks-actions |
Follow-on to #2559
This is making me want to write a proc macro 🤔
r? @RalfJung