TreeBorrows: Put accesses diagnostic parameters into a single struct #4740
Conversation
|
Thank you for contributing to Miri! A reviewer will take a look at your PR, typically within a week or two. |
| /// Diagnostics data about the current access and the location we are accessing. | ||
| /// Used to create history events and errors. | ||
| #[derive(Clone, Debug)] | ||
| pub struct DiagnosticsExtra { |
There was a problem hiding this comment.
Maybe it should be more something like AccessContext? It's just a bunch of metadata about the current access.
There was a problem hiding this comment.
I like have "Diag(nostics)" in the name as that makes it purpose more clear.
| span: Span, //diagnostics | ||
| location_range: Range<u64>, //diagnostics | ||
| protected: bool, | ||
| diagnostics: &DiagnosticsExtra, |
There was a problem hiding this comment.
FWIW in Stacked Borrows we have a lot of dcx: &mut DiagnosticCx<'_, '_, 'tcx>; would that also make sense here? (apparently mutability is currently not needed in TB)
There was a problem hiding this comment.
I don't think mutability is necessary. We could also pass it by value.
There was a problem hiding this comment.
Passing it by-ref is good, both for performance and to document that this isn't changing.
| alloc_id, | ||
| span, | ||
| ChildrenVisitMode::VisitChildrenOfAccessed, | ||
| &DiagnosticsExtra { |
There was a problem hiding this comment.
Please let-bind this before the call.
| alloc_id, | ||
| span, | ||
| ChildrenVisitMode::SkipChildrenOfAccessed, | ||
| &DiagnosticsExtra { |
3 participants
All the
perform_accessfunctions take 5 parameters that are purely used for diagnostic purposes. This puts them all into a single struct and passes them by reference.