Conversation
|
I think the "new" |
Brandhoej
left a comment
There was a problem hiding this comment.
Some minor requests. Personally, I think that the rename from Guard to Expression was necessary as it made the cod more clear as invariants should not be type Guard. However, I think we should revisit the structure of this AST. Should we instead of AndExpression, OrExpression, and BoolExpression have a BinaryExpression class. Instead of TrueExpression and FalseExpression we should consider Literal with boolean value and integeger for boolValue in BoolExpression. Then we could have classes for Guard and Invariant. Making this change would follow a more conventional structure. For this reason, I think an issue on this should be opened and act as a dependency for #97.
src/models/AndExpression.java
Outdated
|
|
||
| @Override | ||
| public String prettyPrint() { | ||
| return Expression.compositePrettyPrint(expressions, "&&"); |
There was a problem hiding this comment.
I think this call does exactly the same as toString.
src/models/OrExpression.java
Outdated
|
|
||
| @Override | ||
| public String prettyPrint() { | ||
| return Expression.compositePrettyPrint(expressions, "||"); |
There was a problem hiding this comment.
Ìf toString joins on || instead of or then I think they are exactly the same
src/models/Expression.java
Outdated
| import java.util.stream.Collectors; | ||
|
|
||
| public abstract class Guard { | ||
| public abstract class Expression { |
There was a problem hiding this comment.
Can you please add some documentation to this class?
|
I am afraid I am misunderstanding, so I have a few questions in regards to the new structure you are suggesting: In regards to the grammar ( boolExpr : VARIABLE EQ BOOLEAN | VARIABLE operator INT;
operator : EQ | ORD ;
EQ : ('==' | '!=')
ORD: ('>=' | '<=' | '<' | '>')This may also let us omit the check in if (relation != Relation.EQUAL && relation != Relation.NOT_EQUAL) {
throw new IllegalArgumentException("The relation of the clock expression is invalid");
} |
Fixes #80
This PR is more or less just a big refactor. I have tried to rename
GuardtoExpressionandGuardtoInvariantwhen it fits (Expressionin general, andInvariantin context of locations). However, I have a few places I am uncertain if I should change or notsrc/logic/State.java l. 43- We have these identical functions. Since it is aState, I am not sure whether it is a guard, an inviriant, or just an expression we are applying.src/models/Federation.java l. 51- Just to be sure, it should be the more general termExpressionused here instead ofGuard? Or maybe just remove the function as it is currently unused (unless it will be used in the future)?src/models/Zone.java l. 53- There are a lot of occurences ofGuardin this file, but I am mainly thinking of the methodbuildConstraintsForGuard, it this truly only happens when building guards on edges, or if this should be renamed to something more general.Expressionis to ambiguous, since it could be interpreted as both arithmetic and boolean expressions. Should It be renamed toBoolExpressionor something similar? And how should we then handle the distinction between the currentBoolExpressionand the "new"?