-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Interpret: AllocRange Debug impl, and use it more consistently #98811
Conversation
Some changes occurred to the CTFE / Miri engine cc @rust-lang/miri Some changes occurred to the CTFE / Miri engine cc @rust-lang/miri |
self.read_scalar(alloc_range(offset, size), /*read_provenance*/ false) | ||
/// `range` is relative to this allocation reference, not the base of the allocation. | ||
pub fn read_integer(&self, range: AllocRange) -> InterpResult<'tcx, ScalarMaybeUninit<Tag>> { | ||
self.read_scalar(range, /*read_provenance*/ false) |
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.
@oli-obk I know you said you didn't like these alloc_range(...)
calls. OTOH, I tried changing all these functions to offset-size pairs, and it has the usual problems of a function where two arguments have the same type -- call sites become rather ambiguous-looking:
alloc.write_scalar(Size::ZERO, a_size, a_val)?;
This is not an access of size zero.
By using alloc_range
, at least one has to only learn once what that function does. I originally intended to use this as AllocRange { start: Size::ZERO, size: a_size }
, which is very clear but also very verbose. I wish we could write _ { start: Size::ZERO, size: a_size }
and have rustc infer the type... ;)
I am open to other ideas.
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 recently saw a suggestion for start..+len
, while neat, I find it an extension to the language that definitely does not carry its weight. The additional sigils aren't very obvious or googleable.
for slices we at least have x[start..][..len]
. If indexing could return non-references we could go this route. We could use a method, which is likely not worse than what we have:
let align = vtable
.read_integer(alloc_range(
pointer_size * u64::try_from(COMMON_VTABLE_ENTRIES_ALIGN).unwrap(),
pointer_size,
))?
.check_init()?;
could become
let align = vtable
.offset(pointer_size * u64::try_from(COMMON_VTABLE_ENTRIES_ALIGN).unwrap())
.with_len(pointer_size)
.read_integer()?
.check_init()?;
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.
fwiw, I think my main complaint is that read_scalar
and friends take an AllocRange
argument instead of first calling a method to offset/shrink the alloc and then just having no positional arguments to read_scalar
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 recently saw a suggestion for start..+len, while neat, I find it an extension to the language that definitely does not carry its weight. The additional sigils aren't very obvious or googleable.
We could have a macro, alloc_range!(start..+len)
? Though not sure if the parser will allow tat.
fwiw, I think my main complaint is that read_scalar and friends take an AllocRange argument instead of first calling a method to offset/shrink the alloc and then just having no positional arguments to read_scalar
Oh, interesting idea; I had not considered that. But what argument would the restrict-range function take? Another AllocRange
? Or does offset
change the start without changing the end (i.e., it changes both start and size, the way things are currently represented)?
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.
Or does
offset
change the start without changing the end (i.e., it changes both start and size, the way things are currently represented)?
yea, this. Could probably use a better name
This comment has been minimized.
This comment has been minimized.
0802113
to
d31cbb5
Compare
Co-authored-by: Joe ST <[email protected]>
@bors r+ |
📌 Commit 0832d1d has been approved by |
…li-obk Interpret: AllocRange Debug impl, and use it more consistently The two commits are pretty independent but it did not seem worth having two PRs for them. r? `@oli-obk`
Rollup of 6 pull requests Successful merges: - rust-lang#97300 (Implement `FusedIterator` for `std::net::[Into]Incoming`) - rust-lang#98761 (more `need_type_info` improvements) - rust-lang#98811 (Interpret: AllocRange Debug impl, and use it more consistently) - rust-lang#98847 (fix interpreter validity check on Box) - rust-lang#98854 (clean up the borrowing in rustc_hir_pretty) - rust-lang#98873 (Suggest `#[derive(Default)]` to enums with `#[default]`) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Bump rust version rustc changes: rust-lang/rust#98811 rustc issue: rust-lang/rust#98922
The two commits are pretty independent but it did not seem worth having two PRs for them.
r? @oli-obk