Turn Datum into an enum - #105
Open
Twisol wants to merge 3 commits into
Open
Conversation
Using an enum instead of a struct makes it impossible to use the wrong field, and allows us to couple the tag-based dispatch with uses of those fields. This pays off significantly in type coercions for each operator, where some new helper functions have been used to unify the coercion and evaluation blocks.
According to the language reference, >> is guaranteed to be an arithmetic (sign-preserving) shift. https://doc.rust-lang.org/reference/expressions/operator-expr.html#arithmetic-and-logical-binary-operators
fogti
reviewed
Sep 18, 2021
| flt: 0.0, | ||
| str: String::new(), | ||
| } | ||
| Datum::String("".to_owned()) |
There was a problem hiding this comment.
Suggested change
| Datum::String("".to_owned()) | |
| Datum::String(String::new()) |
fogti
reviewed
Sep 18, 2021
Comment on lines
49
to
55
| pub(crate) fn int(int: MoltInt) -> Self { | ||
| Self { | ||
| vtype: Type::Int, | ||
| int, | ||
| flt: 0.0, | ||
| str: String::new(), | ||
| } | ||
| Datum::Int(int) | ||
| } | ||
|
|
||
| pub(crate) fn float(flt: MoltFloat) -> Self { | ||
| Self { | ||
| vtype: Type::Float, | ||
| int: 0, | ||
| flt, | ||
| str: String::new(), | ||
| } | ||
| Datum::Float(flt) | ||
| } |
There was a problem hiding this comment.
These methods don't really contain any useful functionality anymore, it might be a good idea to replace any calls to them with direct invocations of the enum variant constructors
fogti
reviewed
Sep 18, 2021
Comment on lines
+240
to
244
| match value { | ||
| Datum::Int(v) => molt_ok!(Value::from(v)), | ||
| Datum::Float(v) => molt_ok!(Value::from(v)), | ||
| Datum::String(v) => molt_ok!(Value::from(v)), | ||
| } |
There was a problem hiding this comment.
I'm not sure if this would work, but it might be worth trying
Suggested change
| match value { | |
| Datum::Int(v) => molt_ok!(Value::from(v)), | |
| Datum::Float(v) => molt_ok!(Value::from(v)), | |
| Datum::String(v) => molt_ok!(Value::from(v)), | |
| } | |
| molt_ok!(match value { | |
| Datum::Int(v) => Value::from(v), | |
| Datum::Float(v) => Value::from(v), | |
| Datum::String(v) => Value::from(v), | |
| }) |
fogti
reviewed
Sep 18, 2021
Comment on lines
-664
to
+540
| if value.int < 0 { | ||
| value.int = !((!value.int) >> value2.int) | ||
| } else { | ||
| value.int >>= value2.int; | ||
| } | ||
| value = Datum::Int(v1 >> v2) |
There was a problem hiding this comment.
It might be a good idea to create a unit test which checks this assumption.
There was a problem hiding this comment.
oh, if it is explicitly mentioned in the rust manual that won't be necessary.
fogti
reviewed
Sep 18, 2021
Comment on lines
+543
to
547
| value = match coerce_pair(operator, &value, &value2)? { | ||
| DatumPair::Ints(v1, v2) => Datum::Int((v1 < v2) as MoltInt), | ||
| DatumPair::Floats(v1, v2) => Datum::Int((v1 < v2) as MoltInt), | ||
| DatumPair::Strings(v1, v2) => Datum::Int((v1 < v2) as MoltInt), | ||
| }; |
There was a problem hiding this comment.
Suggested change
| value = match coerce_pair(operator, &value, &value2)? { | |
| DatumPair::Ints(v1, v2) => Datum::Int((v1 < v2) as MoltInt), | |
| DatumPair::Floats(v1, v2) => Datum::Int((v1 < v2) as MoltInt), | |
| DatumPair::Strings(v1, v2) => Datum::Int((v1 < v2) as MoltInt), | |
| }; | |
| value = Datum::Int(match coerce_pair(operator, &value, &value2)? { | |
| DatumPair::Ints(v1, v2) => v1 < v2, | |
| DatumPair::Floats(v1, v2) => v1 < v2, | |
| DatumPair::Strings(v1, v2) => v1 < v2, | |
| } as MoltInt); |
fogti
reviewed
Sep 18, 2021
Comment on lines
+550
to
554
| value = match coerce_pair(operator, &value, &value2)? { | ||
| DatumPair::Ints(v1, v2) => Datum::Int((v1 > v2) as MoltInt), | ||
| DatumPair::Floats(v1, v2) => Datum::Int((v1 > v2) as MoltInt), | ||
| DatumPair::Strings(v1, v2) => Datum::Int((v1 > v2) as MoltInt), | ||
| }; |
There was a problem hiding this comment.
Suggested change
| value = match coerce_pair(operator, &value, &value2)? { | |
| DatumPair::Ints(v1, v2) => Datum::Int((v1 > v2) as MoltInt), | |
| DatumPair::Floats(v1, v2) => Datum::Int((v1 > v2) as MoltInt), | |
| DatumPair::Strings(v1, v2) => Datum::Int((v1 > v2) as MoltInt), | |
| }; | |
| value = Datum::Int(match coerce_pair(operator, &value, &value2)? { | |
| DatumPair::Ints(v1, v2) => v1 > v2, | |
| DatumPair::Floats(v1, v2) => v1 > v2, | |
| DatumPair::Strings(v1, v2) => v1 > v2, | |
| } as MoltInt); |
fogti
reviewed
Sep 18, 2021
Comment on lines
+1281
to
+1285
| if v < 0.0 { | ||
| Ok(Datum::Int((v - 0.5) as MoltInt)) | ||
| } else { | ||
| Ok(Datum::Int((v + 0.5) as MoltInt)) | ||
| } |
There was a problem hiding this comment.
Suggested change
| if v < 0.0 { | |
| Ok(Datum::Int((v - 0.5) as MoltInt)) | |
| } else { | |
| Ok(Datum::Int((v + 0.5) as MoltInt)) | |
| } | |
| Ok(Datum::Int(if v < 0.0 { | |
| v - 0.5 | |
| } else { | |
| v + 0.5 | |
| } as MoltInt)) |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Hi Will -- it's Jonathan! This weekend, I'm making good on my years-old promise to check out Molt. Feel free to take or leave the changes in this PR; I had an idea to try out and it seems to have worked fairly nicely, but it's up to your discretion in the end.
This PR turns the existing
struct Datuminto a more Rust-yenum Datum, and propagates those changes throughoutexpr.rs. In the process, I ended up unifying the type coercion sections for each operator with their respective evaluation sections. I especially noticed an inconsistency between the assumptions made by execution forANDandORand what the respective coercion was doing; I think the approach in this PR scopes the relevant assumptions much more tightly, so it'll be much more difficult for things to get out of sync like that.This PR also contains minor contributions for the shift-right operator and absolute value function, which can both be simplified by using core Rust functionality. (Yes, as an arithmetic shift,
>>is guaranteed to preserve sign!)Lastly, I ran
rustfmton my changes to keep things consistent. I find some of the formatting choices it made to be less readable, but I defer to convention :)