Guard rename #95

t-lohse · 2022-12-01T10:49:52Z

Fixes #80
This PR is more or less just a big refactor. I have tried to rename Guard to Expression and Guard to Invariant when it fits (Expression in general, and Invariant in context of locations). However, I have a few places I am uncertain if I should change or not

src/logic/State.java l. 43 - We have these identical functions. Since it is a State, I am not sure whether it is a guard, an inviriant, or just an expression we are applying.

public void applyGuards(CDD guardCDD) {
    invarCDD = invarCDD.conjunction(guardCDD) ;
}

public void applyInvariants(CDD invarCDD) {
    this.invarCDD = this.invarCDD.conjunction(invarCDD);
}

src/models/Federation.java l. 51 - Just to be sure, it should be the more general term Expression used here instead of Guard? Or maybe just remove the function as it is currently unused (unless it will be used in the future)?

public Expression toGuards(List<Clock> clocks)

src/models/Zone.java l. 53 - There are a lot of occurences of Guard in this file, but I am mainly thinking of the method buildConstraintsForGuard, it this truly only happens when building guards on edges, or if this should be renamed to something more general.

public void buildConstraintsForGuard(ClockExpression guard, List<Clock> clocks) {
    if (guard.isDiagonal()) {
        buildConstraintsForDiagonalConstraint(guard, clocks);
    } else {
        buildConstraintsForNormalGuard(guard, clocks);
    }
}

And finally, I am generally unsure if Expression is to ambiguous, since it could be interpreted as both arithmetic and boolean expressions. Should It be renamed to BoolExpression or something similar? And how should we then handle the distinction between the current BoolExpression and the "new"?

Brandhoej · 2022-12-19T11:49:55Z

src/logic/State.java l. 43: We could merge the to functions and name it applyToInvariant. Then the formal parameter could be named expression.

src/models/Federation.java l. 51: Since it calls buildExpressionFromZone (I would prefer it to be called toExpression, as it does not apply the builder pattern) then I think an appropriate name could be toExpression

src/models/Zone.java l. 53: buildConstraintsForGuard should maybe be called applyConstraints.

I think the "new" Expression should be renamed to BooleanExpression. Then the "old" BooleanExpression should be changed to something like BinaryBooleanExpression.

Brandhoej

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/logic/AggregatedTransitionSystem.java

src/logic/State.java

src/logic/Transition.java

Brandhoej · 2022-12-19T12:55:14Z

src/models/AndExpression.java

+
+    @Override
+    public String prettyPrint() {
+        return Expression.compositePrettyPrint(expressions, "&&");


I think this call does exactly the same as toString.

src/models/Federation.java

src/models/Move.java

Brandhoej · 2022-12-19T13:00:58Z

src/models/OrExpression.java

+
+    @Override
+    public String prettyPrint() {
+        return Expression.compositePrettyPrint(expressions, "||");


Ìf toString joins on || instead of or then I think they are exactly the same

src/models/Zone.java

Brandhoej · 2022-12-19T13:07:59Z

src/models/Expression.java

@@ -3,11 +3,11 @@
 import java.util.List;
 import java.util.stream.Collectors;

-public abstract class Guard {
+public abstract class Expression {


Can you please add some documentation to this class?

t-lohse · 2023-01-16T11:50:22Z

I am afraid I am misunderstanding, so I have a few questions in regards to the new structure you are suggesting:
Are you saying we should remove AndExpression, OrExpression, and BoolExpression and replace it with a BinaryExpression, or have them derive from this new BinaryExpression class? And why would BoolExpression be this, since it is not composite? And if BinaryExpression does replace the others, would you like it as a "simple" tree-structure?
I am not entirely sure what you mean with the new Guard and Invariant classes, wouldn't they be the same, and just a wrapper around the expressions?
Would you like to have the Literal class derive from BooleanExpression as well, making it easier to have a simple literal without having to use BoolExpression?

In regards to the grammar (src/antlr/ExpressionGrammar.g4), should it be called expression(s) as I have implemented currently, or should they be renamed as well? If it should be changed, should we rename boolExpr as well to avoid naming clash?
Beyond that, I noticed that boolExpr is defined as boolExpr : VARIABLE OPERATOR BOOLEAN, which means that the expression x < true is valid. I don't know if this is the point and I just have misunderstood something in regards to the expressions, or if this is a mistake and should be boolExpr : VARIABLE OPERATOR INT or limit the operators such as

boolExpr : VARIABLE EQ BOOLEAN | VARIABLE operator INT;

operator : EQ | ORD ;
EQ : ('==' | '!=')
ORD: ('>=' | '<=' | '<' | '>')

This may also let us omit the check in src/models/BoolExpression.java l. 12:

if (relation != Relation.EQUAL && relation != Relation.NOT_EQUAL) {
    throw new IllegalArgumentException("The relation of the clock expression is invalid");
}

t-lohse added 3 commits December 1, 2022 11:19

Initial rename

53d08fa

Updated CDD and Grammar naming

4bb150a

Updated tests

2a1e197

t-lohse marked this pull request as ready for review December 2, 2022 07:33

Brandhoej added the enhancement New feature or request label Dec 6, 2022

Brandhoej assigned t-lohse Dec 6, 2022

Brandhoej reviewed Dec 19, 2022

View reviewed changes

t-lohse added 3 commits January 16, 2023 10:58

Pull from main

9334d41

Some refactor

a268ca3

Expression rename

d118b15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Guard rename #95

Guard rename #95

t-lohse commented Dec 1, 2022 •

edited

Loading

Brandhoej commented Dec 19, 2022

Brandhoej left a comment

Brandhoej Dec 19, 2022

Brandhoej Dec 19, 2022

Brandhoej Dec 19, 2022

t-lohse commented Jan 16, 2023

Guard rename #95

Are you sure you want to change the base?

Guard rename #95

Conversation

t-lohse commented Dec 1, 2022 • edited Loading

Brandhoej commented Dec 19, 2022

Brandhoej left a comment

Choose a reason for hiding this comment

Brandhoej Dec 19, 2022

Choose a reason for hiding this comment

Brandhoej Dec 19, 2022

Choose a reason for hiding this comment

Brandhoej Dec 19, 2022

Choose a reason for hiding this comment

t-lohse commented Jan 16, 2023

t-lohse commented Dec 1, 2022 •

edited

Loading