change ConstInt impl to not rely on ScalarInt
#96789
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
the
T-compiler
|
Some changes occured to the CTFE / Miri engine cc @rust-lang/miri |
|
r? @oli-obk (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
|
|
The job Click to see the possible cause of the failure (guessed by this bot) |
oli-obk
approved these changes
May 7, 2022
| use std::convert::{TryFrom, TryInto}; | ||
| use std::fmt; | ||
|
|
||
| use crate::ty::TyCtxt; | ||
| #[derive(Copy, Clone)] | ||
| enum IntegerTy { |
There was a problem hiding this comment.
I checked, but there seem to be no existing enums like this. Surprising...
There was a problem hiding this comment.
We might want to refactor Ty to have a single Int variant with a field of this type, rather than 2 top-level Int and Uint variants.
|
|
||
| #[derive(Copy, Clone)] | ||
| /// A type for representing any integer. Only used for printing. | ||
| pub struct ConstInt { | ||
| /// The "untyped" variant of `ConstInt`. | ||
| int: ScalarInt, |
There was a problem hiding this comment.
I thought this was going to get more reuse, looks like it never did
|
This is surprising to me. ScalarInt was created to share the Scalar integer handling with const representation. And now we say we don't want that sharing...? |
RalfJung
reviewed
May 8, 2022
| signed: bool, | ||
| /// Whether the value is a `usize` or `isize` type. | ||
| is_ptr_sized_integral: bool, | ||
| pointer_width: u32, |
There was a problem hiding this comment.
What does this field do/mean? Please add a doc comment to each field.
Note that ConstInt isn't used anywhere but printing. Which is probably why there are no uses of the ScalarInt field. As apparent from the diff, all creation sites of ConstInt had to jump through hoops to get a ScalarInt to feed into the new function |
RalfJung
reviewed
May 8, 2022
| ConstInt::new_with_width( | ||
| int.assert_bits(int.size()), | ||
| self.layout.ty, | ||
| int.size().bytes() as u32, |
There was a problem hiding this comment.
Please don't use as casts in the interpreter.
oli-obk
added
S-waiting-on-author
|
☔ The latest upstream changes (presumably #96883) made this pull request unmergeable. Please resolve the merge conflicts. |
|
I don't really like the current state of this PR and don't have the capacity to work on it (or to work on the reason why I needed it in the first place). Going to close it for now |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This allows us to use it in the thir pattern code in the future without having to first go through
ScalarInt. Patterns currently useu128instead ofScalarIntand moving away from that seems more challenging than this while maybe even impacting perf.