Conversation
EMV2Instance.ecore and EMV2Instance.genmodel were copied from Peter's old EMV2Instance branch.
The EMV2 instantiator also needed to be updated in order for the tests to pass.
This is done to allow generation of emv2 instance elements on the SystemInstance. Previously, these elements were not generated because SystemInstance.getClassifier() returns null.
These subtypes handle the separate cases when a propagation refers to a feature, refers to a propagation point, or has a binding reference.
Renamed FeatureReference, PointReference, and BindingReference to FeaturePropagation, PointPropagation, and BindingPropagation.
The purpose of this restructuring was to create a specific EClass to represent the results of flattening out a type set. This EClass is TypeTokenInstance which is the super type of TypeInstance and TypeProductInstance. TypeSetElement becomes the new super type of TypeTokenInstance and TypeSetInstance. Also created AbstractTypeSet and AnonymousTypeSet. Propagations now point to an AnonymousTypeSet instead of a list of elements. This was done to have a single implementation of flatten in AbstractTypeSet.
getSourcePropagation() and getDestinationPropagation() now both return ConnectionEndPropagation instead of ErrorPropagationInstance.
This is to investigate formatter tests that are failing on the build server, but passing when run locally.
This addresses a problem with test failures during the maven build that did not occur when running the tests from within Eclipse. Since the plugin org.osate.aadl2.errormodel.tests contains both JUnit 4 and 5 tests, this was causing FormatterTest and Issue2398Test to fail during the maven build. Upgrading these two tests seems to have fixed the issue.
Contributor
|
Hi, I'm not sure how to review this -- it "seemed" too large to me, and sure enough a quick google turned up "optimal sizes" of pull requests that are 2-3 orders of magnitude smaller than this one (see, eg, 1, 2, 3). Even granting that much of the 58,000sloc is generated, this is too large for my review. Can it be restructured and split into multiple PRs? Or do you (@lwrage) want to waive my request? |
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
3 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.
Fixes #2802.
This is the updated pull request and branch for the error model instance. The old pull request can be found here: #2646.
This branch started with taking Peter's meta-model, instantiator, tests, and other supporting code and copying it to a new branch. Now that his code runs again, we can begin to evaluate his meta-model.
Here are some low level issues found with the meta-model:
EOperationis poorly named. A more meaningful name should be selected and especially one that does not conflict with a standard EMF type name.ConstraintElement,ConstraintExpression, andConstrainedInstanceObjectare properly named.StateMachineInstancehas the fieldcurrentState. This seems to be similar to howSystemInstancekeeps track of its current SOM. Peter's instantiator setscurrentStateto the current state, but there is no code that updates this field. Do we really want to handle state by mutating the model? Would it be better for an analysis or utility method to iterate and keep track of the current state rather than storing it in the model?ConstrainedInstanceObjecthas the fieldinstanceObjectwhich points to anInstanceObject. The name of this field should be updated to be more descriptive. Also, the type should be more restrictive.ConstrainedInstanceObject.propagationKindhas the typeEString. Can this be a better type such as an enum? I think this is supposed to be set when a propagation doesn't refer to a feature, but instead is marked asprocessor,memory,connection, etc. I think that the field is a string in the declarative because of Xtext restrictions, but the instantiator should be able to convert that to an enum.ConstrainedInstanceObject, it seems that either the fieldinstanceObjectshould be set or the fieldpropagationKindshould be set. It seems like they will never both be set. If this is true, this restriction should be expressed in the meta-model.ConstrainedInstanceObjectandErrorPropagationInstance. Should these be one type or does the inheritance make sense? The fieldpropagationKindis onConstrainedInstanceObject, but it would seem that it should be onErrorPropagationInstance, but it is never set when constructing anErrorPropagationInstance.StateTransitionInstancehas the fieldstateTransitionwhich points to anEObject. The type of this field should be more restrictive.ErrorFlowInstancehas the fieldemv2Elementwhich points to aNamedElement. The name of this field should be updated to be more descriptive. Also, the type should be more restrictive.ErrorFlowInstancehas the fieldssourceandsink, both of which areEBooleans. First of all, these fields are never used. It seems like the intention is thatsourcewill betruefor flow sources,sinkwill betruefor flow sinks, and both will betruefor flow paths. There are so many alternatives which could be better such as an enum, checking the fieldsincomingandoutgoingfornull, or even creating the subtypesErrorFlowSourceInstance,ErrorFlowSinkInstance, andErrorFlowPathInstance.ErrorPropagationConditionInstancehas the fieldemv2Element. The name of this field should be updated to be more descriptive.ErrorPropagationConditionInstancehas the fieldssourceandsink, both of which areEBooleans. I don't think these fields make any sense and they are probably a copy & paste error.ErrorDetectionInstancehas the fieldemv2Element. The name of this field should be updated to be more descriptive.ErrorDetectionInstancehas the fielderrorCodewhich points to anEString. In the declarative, this could be an integer, a string, or a property constant. I'm assuming that the property constant only makes sense when it points to an integer or a string. Currently when instantiating, Peter takes any integers and converts them to strings. Is this correct? Should we make a distinction between an integer and a string in the instance meta-model.ConstraintExpressionhas the fieldkwhich is anELong. This is only used when theoperatorisKORMORE. Should the operator really be an enum or should we have specific subtypes with only the fields that are needed?PropagationPathInstancehas the fieldemv2Elementwhich points to aNamedElement. The name of this field should be updated to be more descriptive. Also, the type should be more restrictive.