Introduce OneToManyValueExpression and OneToOneValueExpression #247
Conversation
| return ImmutableList.create(eval(operand.eval(parseState, encoding), parseState, encoding)); | ||
| } | ||
|
|
||
| public abstract Optional<Value> eval(final ImmutableList<Optional<Value>> list, final ParseState parseState, final Encoding encoding); |
There was a problem hiding this comment.
Self-review: I think we do not need to pass ParseState into this method, we do not use it anywhere.
There was a problem hiding this comment.
| @@ -33,13 +33,13 @@ | |||
| import java.io.IOException; | |||
| import java.util.Optional; | |||
|
|
|||
| import io.parsingdata.metal.expression.value.OneToManyValueExpression; | |||
There was a problem hiding this comment.
Self-review: Fix import order.
| import static io.parsingdata.metal.data.Slice.createFromSource; | ||
| import static java.math.BigInteger.ZERO; |
There was a problem hiding this comment.
Self-review: fix import order?
Codecov Report
@@ Coverage Diff @@
## current-iteration-levels #247 +/- ##
==============================================================
- Coverage 100% 99.78% -0.22%
+ Complexity 1066 1018 -48
==============================================================
Files 92 93 +1
Lines 1430 1387 -43
Branches 144 138 -6
==============================================================
- Hits 1430 1384 -46
- Misses 0 1 +1
- Partials 0 2 +2
Continue to review full report at Codecov.
|
| public static BinaryValueExpression and(final ValueExpression left, final ValueExpression right) { return new io.parsingdata.metal.expression.value.bitwise.And(left, right); } | ||
| public static BinaryValueExpression or(final ValueExpression left, final ValueExpression right) { return new io.parsingdata.metal.expression.value.bitwise.Or(left, right); } | ||
| public static UnaryValueExpression not(final ValueExpression operand) { return new io.parsingdata.metal.expression.value.bitwise.Not(operand); } | ||
| public static OneToManyValueExpression not(final ValueExpression operand) { return new io.parsingdata.metal.expression.value.bitwise.Not(operand); } |
There was a problem hiding this comment.
Referring to your naming debate, reading this does make it seem a bit funny in that it now reads like negate/not have a different cardinality between input and output (consider an identity function, it would be even more weird in my opinion), although both are simply (unbounded) ValueExpressions. Before, Unary meant a single operand, with implicitly a single output value (as always). Now I think it is a mix of two concepts.
But thinking of a good name is hard. UnaryManyToMany to me feels like I would know better what it all is about. But there might be better names to be found. Or I am just thinking about it wrong. 🆘
EDIT: as another example, last being a OneToOne just does not quite feel right to me (many to one?).
There was a problem hiding this comment.
I agree that because of our list semantics, the notion of what is a single operand is a bit complex... We should probably approach this in a more fundamental way: think about the possible types, then the ones that actually occur, then which ones we should implement/support and after all that, how to name them.
There was a problem hiding this comment.
Sounds good :) It is also interesting for expressions with more than one operand, where e.g. mul can have two lists, but exp can have a list as the first operand, but requires a single element as the second.
It would probably be cool to encode that information in the operand types, i.e. BinaryExpr&ToManyExpr exp(ToManyExpr value, ToOneExpr second). It can then allow us to statically check for correctness as well, as per #70 (although there are few cases where it is necessary?). Downside is that we can not pass a list expression where we know that it will only ever contain a single value, so in the exp case, it would need a wrap with a last for example.
And there are probably other things to consider.
|
The cleanup bit is nice I think! |
|
Closing this PR for now, we'll keep it as reference for issue #246 and others. |
This removes duplicated code in these
ValueExpressions:Last,First,CountandFoldCat. This is achieved by introducing a new abstract base classOneToOneValueExpression. The four classes look pretty clean now!The old
UnaryValueExpressionhas been renamed toOneToManyValueExpression.The names of these classes are still open to debate, not too sure if these are the best names.
I found that the
Bytesclass still contains this exact code, but is not a match for either base class, since we "expand" each result of the operand into multiple values.