Implement operations on typed values #293
Conversation
|
This seems reasonable to me. |
philipc
force-pushed
the
op-types
branch
2 times, most recently
from
43a7b83
to
2dabeb0
Compare
philipc
changed the title
|
This is ready for review now. |
src/value.rs
Outdated
| pub enum ValueType { | ||
| /// The generic type, which is address-sized and of unspecified sign. | ||
| /// This type is also used to represent address base types. | ||
| Generic, |
There was a problem hiding this comment.
Maybe point to 2.5.1. in dwarf5 here as well?
| /// | ||
| /// The `ValueType` of `self` must be integral. | ||
| /// Values are sign extended if the source value is signed. | ||
| pub fn to_u64(self, addr_mask: u64) -> Result<u64> { |
There was a problem hiding this comment.
We should file a follow up issue to implement TryFrom<Value> for u64 in a month and a half, when it hits stable.
There was a problem hiding this comment.
I don't think that allows the addr_mask parameter.
| /// The `value_type` may be integral or floating point. | ||
| /// The result is truncated if the `u64` value does | ||
| /// not fit the bounds of the `value_type`. | ||
| pub fn from_u64(value_type: ValueType, value: u64) -> Result<Value> { |
There was a problem hiding this comment.
And here we also need the value_type parameter. I guess you could implement TryFrom<(ValueType, u64)>, but I think the function call is clearer.
| /// The `value_type` may be integral or floating point. | ||
| /// The result is not defined if the `f64` value does | ||
| /// not fit the bounds of the `value_type`. | ||
| fn from_f64(value_type: ValueType, value: f64) -> Result<Value> { |
There was a problem hiding this comment.
And I guess all of these variants.
| (Value::I32(v1), Value::I32(v2)) => Value::I32(v1.wrapping_add(v2)), | ||
| (Value::U32(v1), Value::U32(v2)) => Value::U32(v1.wrapping_add(v2)), | ||
| (Value::I64(v1), Value::I64(v2)) => Value::I64(v1.wrapping_add(v2)), | ||
| (Value::U64(v1), Value::U64(v2)) => Value::U64(v1.wrapping_add(v2)), |
There was a problem hiding this comment.
I feel like all these operation implementations could use a macro or something to reduce boilerplate.
There was a problem hiding this comment.
What do you think of this: philipc@d0b80cf?
| (Value::I64(v1), Value::I64(v2)) => v1 >= v2, | ||
| (Value::U64(v1), Value::U64(v2)) => v1 >= v2, | ||
| (Value::F32(v1), Value::F32(v2)) => v1 >= v2, | ||
| (Value::F64(v1), Value::F64(v2)) => v1 >= v2, |
There was a problem hiding this comment.
Thinking about it a little more, something like this could be useful:
enum SameTypeValues {
Generic(u64, u64),
I8(i8, i8),
U8(u8, u8),
I16(i16, i16),
U16(u16, u16),
I32(i32, i32),
U32(u32, u32),
I64(i64, i64),
U64(u64, u64),
F32(f32, f32),
F64(f64, f64),
}
impl SameTypeValues {
fn try_from(a: Value, b: Value) -> Result<SameTypeValues> {
match (a, b) {
(Generic(x), Generic(y)) => Ok(SameTypeValues::Generic(x, y)),
(I8(x), I8(y)) => Ok(SameTypeValues::I8(x, y)),
(U8(x), U8(y)) => Ok(SameTypeValues::U8(x, y)),
(I16(x), I16(y)) => Ok(SameTypeValues::I16(x, y)),
(U16(x), U16(y)) => Ok(SameTypeValues::U16(x, y)),
(I32(x), I32(y)) => Ok(SameTypeValues::I32(x, y)),
(U32(x), U32(y)) => Ok(SameTypeValues::U32(x, y)),
(I64(x), I64(y)) => Ok(SameTypeValues::I64(x, y)),
(U64(x), U64(y)) => Ok(SameTypeValues::U64(x, y)),
(F32(x), F32(y)) => Ok(SameTypeValues::F32(x, y)),
(F64(x), F64(y)) => Ok(SameTypeValues::F64(x, y)),
_ => Err(Error::UnsupportedTypeOperation),
}
}
}There was a problem hiding this comment.
The users of SameTypeValues would still have to match on just as many cases though?
A couple of things of note:
EvaluationResult::RequiresBaseType. Even though this probably isn't something that will need a Future, I chose this because it avoids adding a type parameter toEvaluationfor a closure or trait.I used @tromey's work as a starting point, but the implementation has significantly changed. I'm now using an enum for the value, instead of a struct with type and value members. The reason for this is I think it makes it easier to get the operations correct in some cases. For example, I think that division for unsigned base types should be an unsigned division (I think the requirement for signed division in the standard only applies to the generic type). One drawback of this approach is that the reinterpret operation is ugly, and I'm open to ideas on how to simplify that. @tromey Can you please take a quick look at this approach and let me know if you see any design problems with it?