Fixes to Merging algorithm #54
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
@padupr What's the current status of this PR? |
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
# Conflicts: # include/vara/Feature/FeatureTreeNode.h # unittests/resources/CMakeLists.txt
Co-authored-by: Florian Sattler <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
|
What is the current state of this PR? @padupr could you please, if you find time, go over the open remarks and close them if they are resolved. |
|
@padupr I pulled in the current fixes on dev. Otherwise, we need to remove the issues reported from the CI. |
|
@s9latimm I updated the PR to our ne dev state. Can you please double check this PR, especially the TODO part regarding error handling. What should we do in this case? Do we need a specific error here? |
Codecov Report
@@ Coverage Diff @@
## vara-dev #54 +/- ##
============================================
- Coverage 88.48% 88.05% -0.44%
============================================
Files 48 48
Lines 5282 5569 +287
============================================
+ Hits 4674 4904 +230
- Misses 608 665 +57
Continue to review full report at Codecov.
|
It currently seems that removing the constraint from a parent seems wrong. However, there might be a case we miss here. So, should we encounter a breakages from that, we should revisit the impl. here.
|
|
@boehmseb @Kallistos please have a look. I tried to rebase and clean up the work from Pascal but an additional feedback would be good. |
| @@ -254,6 +258,14 @@ class FeatureModelModification { | |||
| return FM.addConstraint(std::move(Constraint)); | |||
| } | |||
|
|
|||
| static void removeConstraint(FeatureModel &FM, Constraint *R) { | |||
There was a problem hiding this comment.
Why Consraint *R here and Constraint *C below?
Do they have different meaning, or should they be named the same?
| return UniC->constraint() == C; | ||
| }); | ||
| if (MCIt != MixedConstraints.end()) { | ||
|
|
| if (!Parent->getChildren<Relationship>(1).empty()) { | ||
| ParentNode = *(Parent->getChildren<Relationship>(1).begin()); | ||
| } |
There was a problem hiding this comment.
Just to make sure I understand this correctly:
This is required because we model (xor/or) relationships as separate nodes so the relationship node has to be set as parent if one exists, correct?
This assumes that a Feature can have exactly one (or no) relationship as child and if it has, there are no features as direct children allowed. Is this assumption validated anywhere?
I'm asking because this might become an issue when integrating UVL because UVL does not have these constraints, i.e., a feature can have multiple types of relationships as children at once (I'm not sure how about multiple instances of the same type, e.g., multiple or groups).
Last minute fixes.