635 Add support for parameterized activity to the uml2cif translators#637
635 Add support for parameterized activity to the uml2cif translators#637
Conversation
|
I've marked this as WIP. It is not done yet. |
|
Ready for review |
...yoke.transform.flatten/src/com/github/tno/pokayoke/transform/flatten/FlattenUMLActivity.java
Outdated
Show resolved
Hide resolved
...nsform.flatten/src/com/github/tno/pokayoke/transform/flatten/TemplateParameterFlattener.java
Outdated
Show resolved
Hide resolved
...nsform.flatten/src/com/github/tno/pokayoke/transform/flatten/TemplateParameterFlattener.java
Outdated
Show resolved
Hide resolved
...ke.transform.uml2cif/src/com/github/tno/pokayoke/transform/uml2cif/ModelToCifTranslator.java
Show resolved
Hide resolved
...nsform.flatten/src/com/github/tno/pokayoke/transform/flatten/TemplateParameterFlattener.java
Show resolved
Hide resolved
| import com.google.common.base.Verify; | ||
|
|
||
| /** Template parameter flattener. */ | ||
| public class TemplateParameterFlattener { |
There was a problem hiding this comment.
Since I see quite a big overlap with CompositeDataTypeFlattener, I wonder if we should have some more structure between these two files. Maybe having a Flattener class, that includes activities, composite data types, and templates. Perhaps to be discussed with Dennis Hendriks and/or Wytse.
There was a problem hiding this comment.
The similarity is largely by design; The two classes traverse the parse trees in a very similar manner. Ordinarily I'd argue we should use the CifObjectWalker or CifObjectVisitor (or I misunderstood the purpose of the Flattener?). In my opinion we should look into improving those classes to simplify manipulating the parse tree, right now using those two classes for generic traversal adds complexity rather than reduce it.
...yoke.transform.flatten/src/com/github/tno/pokayoke/transform/flatten/FlattenUMLActivity.java
Show resolved
Hide resolved
...nsform.flatten/src/com/github/tno/pokayoke/transform/flatten/TemplateParameterFlattener.java
Outdated
Show resolved
Hide resolved
...yoke.transform.flatten/src/com/github/tno/pokayoke/transform/flatten/FlattenUMLActivity.java
Show resolved
Hide resolved
|
I couldn't easily parse the new regression tests. Can you please past here a couple of pictures of the activity diagrams before/after of the new template flattener tests? |
Co-authored-by: AndreaPuffo <46629578+AndreaPuffo@users.noreply.github.com>
|
Ready for next review. All comments should be resolved, but I've opted to leave them open to make the discussion easier to see. |
|
Looks good to me. Maybe also @dhendriks wants to have a look. |
|
It's on my list to take a look. |
| if (context.hasParameterizedActivities()) { | ||
| throw new RuntimeException("Translating parameterized activities to CIF is unsupported."); | ||
| } |
There was a problem hiding this comment.
I'd restore this check, after the conditional call to the flattener. If we flatten, there should be no parameterized activities left. If we don't, there should also not be any (already flattened before). It then states the invariant as a sanity check.
| @@ -72,9 +81,21 @@ private void transform(Element element) { | |||
| if (element instanceof Activity activityElement) { | |||
There was a problem hiding this comment.
transform/destroyParameterizedActivities: I dislike the 'activityElement' etc names. It sounds like they are elements of an activity, while it is the activity itself. So, name them 'activity', 'class' (or rather 'cls' to prevent using Java keyword), etc.
| // considered leaves, as are call behavior actions that call opaque behaviors. | ||
| if (behavior instanceof Activity activity && action.getAppliedStereotypes().isEmpty()) { | ||
| if (behavior instanceof Activity activity && action.getAppliedStereotypes().stream() | ||
| .filter(s -> !PokaYokeUmlProfileUtil.FORMAL_CALL_BEHAVIOR_ACTION_STEREOTYPE |
There was a problem hiding this comment.
This checks if no stereotype other than the formal call behavior action stereotype is present. It should instead check for that stereotype not being present, right? So, remove the ! operator? How come this works now, and the tests don't show this issue?
| // considered leaves, as are call behavior actions that call opaque behaviors. | ||
| if (behavior instanceof Activity activity && action.getAppliedStereotypes().isEmpty()) { | ||
| if (behavior instanceof Activity activity && action.getAppliedStereotypes().stream() | ||
| .filter(s -> !PokaYokeUmlProfileUtil.FORMAL_CALL_BEHAVIOR_ACTION_STEREOTYPE |
There was a problem hiding this comment.
Can't we add a method like PokaYokeUmlProfileUtil.isFormalElement to check for this, rather than doing the name check?
| AExpression unfoldedOutgoing = unfoldAExpression(outgoingGuard, nameToArgument); | ||
| PokaYokeUmlProfileUtil.setOutgoingGuard(controlEdge, ACifObjectToString.toString(unfoldedOutgoing)); | ||
| } | ||
| } else if (ownedElement instanceof CallBehaviorAction callBehavior) { |
There was a problem hiding this comment.
Assert that the call behavior has no arguments, as then something is wrong in the flattening order?
| } else if (ownedElement instanceof OpaqueAction internalAction) { | ||
| unfoldRedefinableElement(internalAction, nameToArgument); | ||
| } else if (ownedElement instanceof Constraint) { | ||
| // Constraints cannot be parameterized. |
There was a problem hiding this comment.
| // Constraints cannot be parameterized. | |
| // Constraints cannot refer to activity parameters. |
This PR works towards #635.
The all lower case folder names for tests were inspired by petrify2uml
The TemplateParameterFlattener is a simpler implementation of the CompositeDataTypeFlattener. The regression tests happen in the activity flattener since flattening call behaviors happens during activity flattening and can hardly be taken separately in a full transformation.
To prevent templated activities from containing references to template parameters that cannot be represented down the synthesis chain all templated activities are removed after the flattening process.