-
Notifications
You must be signed in to change notification settings - Fork 265
add test for fields of references #567
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
Conversation
pdg/src/snapshots/c2rust_pdg__tests__analysis_test_pdg_snapshot.snap
Outdated
Show resolved
Hide resolved
pdg/src/snapshots/c2rust_pdg__tests__analysis_test_pdg_snapshot.snap
Outdated
Show resolved
Hide resolved
pdg/src/snapshots/c2rust_pdg__tests__analysis_test_pdg_snapshot.snap
Outdated
Show resolved
Hide resolved
@@ -241,10 +241,12 @@ impl<'tcx> Visitor<'tcx> for InstrumentationAdder<'_, 'tcx> { | |||
statement_index: location.statement_index + 1, | |||
..location | |||
}; | |||
|
|||
let source = remove_outer_deref(*p, self.tcx()); |
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.
remove_outer_deref(*p, self.tcx())
is already computed above on line 238:
&& place_ty(&remove_outer_deref(*p, self.tcx())).is_region_ptr() =>
Could you de-duplicate this?
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.
Maybe a try_remove_outer_deref
would be useful here:
/// Try to strip the initital [`Deref`](ProjectionElem::Deref)
/// from a [`projection`](PlaceRef::projection) sequence.
pub fn try_remove_outer_deref<'tcx>(p: Place<'tcx>, tcx: TyCtxt<'tcx>) -> Option<Place<'tcx>> {
// Remove outer deref if present
match p.as_ref() {
PlaceRef {
local,
projection: &[ref base @ .., ProjectionElem::Deref],
} => Some(Place {
local,
projection: tcx.intern_place_elems(base),
}),
_ => None,
}
}
/// Strip the initital [`Deref`](ProjectionElem::Deref)
/// from a [`projection`](PlaceRef::projection) sequence
/// if there is one.
pub fn remove_outer_deref<'tcx>(p: Place<'tcx>, tcx: TyCtxt<'tcx>) -> Place<'tcx> {
try_remove_outer_deref(p, tcx).unwrap_or(p)
}
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.
Could do
Rvalue::AddressOf(_, p)
if let Some(source) = try_remove_outer_deref(*p, self.tcx())
&& place_ty(source).is_region_ptr() =>
{
// Instrument which local's address is taken
self.loc(location.successor_within_block(), copy_fn)
.arg_var(dest)
.source(source)
.dest(&dest)
.debug_mir(location)
.add_to(self);
}
but that requires #![feature(if_let_guard, let_chains)]
. It might be too clunky without those, I think.
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.
How about the changes in #573?
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 #573 is probably too big for the scope of this PR, but keep it in my mind for after merging this PR.
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.
See my improvement PRs.
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.
Approved conditional on fixing the calloc(0, ...)
in the test case.
…t-end Move the new test to the end so the snapshot isn't disturbed as much
No description provided.