Fixes to Merging algorithm

include/vara/Feature/FeatureModel.h

@@ -286,6 +286,15 @@ class FeatureModel {
     return Constraints.back().get();
   }
 
+  void removeConstraint(Constraint *C) {
+    // TODO se-sic/VaRA#701 implement tree based comparison
+    Constraints.erase(
+        std::find_if(Constraints.begin(), Constraints.end(),
+                     [C](const std::unique_ptr<Constraint> &UniC) {
+                       return UniC.get() == C;
+                     }));
+  }
+
   /// Delete a \a Feature.
   void removeFeature(Feature &Feature);
 

include/vara/Feature/FeatureModelTransaction.h

@@ -138,6 +138,10 @@ class FeatureModelTransaction
     }
   }
 
+  void removeConstraint(Constraint &RemoveConstraint) {
+    this->removeConstraintImpl(RemoveConstraint);
+  }
+
   void setName(std::string Name) { return this->setNameImpl(std::move(Name)); }
 
   void setCommit(std::string Commit) {
@@ -252,6 +256,14 @@ class FeatureModelModification {
     return FM.addConstraint(std::move(Constraint));
   }
 
+  static void removeConstraint(FeatureModel &FM, Constraint *R) {
+    FM.removeConstraint(R);
+  }
+
+  static void removeConstraint(Feature &F, Constraint *C) {
+    F.removeConstraintNonPreserve(C);
+  }
+
   static void setName(FeatureModel &FM, std::string NewName) {
     FM.setName(std::move(NewName));
   }
@@ -308,8 +320,12 @@ class AddFeatureToModel : public FeatureModelModification {
       return ALREADY_PRESENT;
     }
     if (Parent) {
-      setParent(*InsertedFeature, *Parent);
-      addEdge(*Parent, *InsertedFeature);
+      FeatureTreeNode *ParentNode = Parent;
+      if (!Parent->getChildren<Relationship>(1).empty()) {
+        ParentNode = *(Parent->getChildren<Relationship>(1).begin());
+      }
+      setParent(*InsertedFeature, *ParentNode);
+      addEdge(*ParentNode, *InsertedFeature);
     } else if (FM.getRoot()) {
       setParent(*InsertedFeature, *FM.getRoot());
       addEdge(*FM.getRoot(), *InsertedFeature);
@@ -326,6 +342,46 @@ class AddFeatureToModel : public FeatureModelModification {
   Feature *Parent;
 };
 
+//===----------------------------------------------------------------------===//
+//                       RemoveConstraintFromModel
+//===----------------------------------------------------------------------===//
+
+class RemoveConstraintFromModel : public FeatureModelModification {
+  friend class FeatureModelModification;
+  friend class RemoveFeatureFromModel;
+
+public:
+  Result<FTErrorCode> exec(FeatureModel &FM) override {
+    if (auto E = (*this)(FM); !E) {
+      return E.getError();
+    }
+    return Ok();
+  }
+
+  Result<FTErrorCode> operator()(FeatureModel &FM) {
+    // TODO: Any error handling needed here?
+    assert(RemoveConstraint.getParent() == nullptr &&
+           "Subtree deletion is not supported");
+    UncoupleVisitor UV;
+    RemoveConstraint.accept(UV);
+    removeConstraint(FM, &RemoveConstraint);
+    return Ok();
+  }
+
+private:
+  RemoveConstraintFromModel(Constraint &RemoveConstraint)
+      : RemoveConstraint(RemoveConstraint) {}
+
+  class UncoupleVisitor : public ConstraintVisitor {
+  public:
+    void visit(PrimaryFeatureConstraint *C) override {
+      removeConstraint(*C->getFeature(), C);
+    }
+  };
+
+  Constraint &RemoveConstraint;
+};
+
 //===----------------------------------------------------------------------===//
 //                          RemoveFeatureFromModel
 //===----------------------------------------------------------------------===//
@@ -371,6 +427,16 @@ class RemoveFeatureFromModel : public FeatureModelModification {
       }
     }
 
+    // TODO (se-passau/VaRA#790): different approches to handle constraints
+    while (!F->constraints().empty()) {
+      Constraint *C = *(F->constraints().begin());
+      while (C->getParent()) {
+        C = C->getParent();
+      }
+      RemoveConstraintFromModel RCFM(*C);
+      RCFM(FM);
+    }
+
     removeEdge(*F->getParent(), *F);
     removeFeature(FM, *F);
     return Ok();
@@ -899,6 +965,13 @@ class FeatureModelCopyTransactionBase {
         std::move(NewConstraint))(*FM);
   }
 
+  void removeConstraintImpl(Constraint &RemoveConstraint) {
+    assert(FM && "FeatureModel is null.");
+
+    FeatureModelModification::makeModification<RemoveConstraintFromModel>(
+        RemoveConstraint)(*FM);
+  }
+
   void setNameImpl(std::string Name) {
     assert(FM && "FeatureModel is null.");
 
@@ -1020,6 +1093,11 @@ class FeatureModelModifyTransactionBase {
             std::move(NewConstraint)));
   }
 
+  void removeConstraintImpl(Constraint &RemoveConstraint) {
+    Modifications.push_back(FeatureModelModification::makeUniqueModification<
+                            RemoveConstraintFromModel>(RemoveConstraint));
+  }
+
   void setNameImpl(std::string Name) {
     assert(FM && "FeatureModel is null.");
 

lib/Feature/FeatureModelTransaction.cpp

@@ -14,6 +14,8 @@ std::unique_ptr<Feature> featureCopy(Feature &F);
 
 bool compareProperties(const Feature &F1, const Feature &F2, bool Strict);
 
+std::optional<Relationship *> getFeatureRelationship(Feature &F);
+
 void addFeature(FeatureModel &FM, std::unique_ptr<Feature> NewFeature,
                 Feature *Parent) {
   auto Trans = FeatureModelModifyTransaction::openTransaction(FM);
@@ -175,7 +177,27 @@ mergeFeatureModels(FeatureModel &FM1, FeatureModel &FM2, bool Strict) {
   // Is there a similar Feature in the original FM
   if (Feature *CMP = FM.getFeature(F.getName())) {
     if (compareProperties(*CMP, F, Strict)) {
-      // similar feature, maybe merge locations
+      // similar feature, maybe add relationship
+      auto FRelationship = getFeatureRelationship(F);
+      auto CMPRelationship = getFeatureRelationship(*CMP);
+      if (FRelationship && CMPRelationship) {
+        // Relationships must match in strict mode
+        if (Strict && (FRelationship.value()->getKind() !=
+                       CMPRelationship.value()->getKind())) {
+          return false;
+        }
+      } else if (FRelationship && !CMPRelationship) {
+        // Relationship is carried over to work around xml format limits
+        if (CMP->getChildren<Feature>().size() < 2) {
+          Trans.addRelationship(FRelationship.value()->getKind(),
+                                F.getName().str());
+        } else if (Strict) {
+          return false;
+        }
+      }
+      // CMPRelationship is already in the new model
+
+      // merge locations
       for (FeatureSourceRange const &FSR : F.getLocations()) {
         if (std::find(CMP->getLocationsBegin(), CMP->getLocationsEnd(), FSR) ==
             CMP->getLocationsEnd()) {
@@ -191,9 +213,13 @@ mergeFeatureModels(FeatureModel &FM1, FeatureModel &FM2, bool Strict) {
       return false;
     }
     Trans.addFeature(std::move(Copy), F.getParentFeature());
+    // add relationship to new feature
+    auto Relationship = getFeatureRelationship(F);
+    if (Relationship.has_value()) {
+      Trans.addRelationship(Relationship.value()->getKind(), F.getName().str());
+    }
   }
 
-  // copy children if missing
   for (Feature *Child : F.getChildren<Feature>()) {
     if (!mergeSubtree(Trans, FM, *Child, Strict)) {
       return false;
@@ -227,9 +253,11 @@ mergeFeatureModels(FeatureModel &FM1, FeatureModel &FM2, bool Strict) {
 
 [[nodiscard]] bool compareProperties(const Feature &F1, const Feature &F2,
                                      bool Strict) {
+  // name equality
   if (F1.getName() != F2.getName()) {
     return false;
   }
+  // parent equality, properties checked before so name equality is sufficient
   if (F1.getKind() != Feature::FeatureKind::FK_ROOT &&
       F2.getKind() != Feature::FeatureKind::FK_ROOT) {
     if (*(F1.getParentFeature()) != *(F2.getParentFeature())) {
@@ -250,9 +278,6 @@ mergeFeatureModels(FeatureModel &FM1, FeatureModel &FM2, bool Strict) {
   if (F1.getKind() == Feature::FeatureKind::FK_ROOT) {
     return true;
   }
-  if (F1.getParent()->getKind() != F2.getParent()->getKind()) {
-    return false;
-  }
   if (F1.getKind() == Feature::FeatureKind::FK_BINARY) {
     return true;
   }
@@ -264,4 +289,14 @@ mergeFeatureModels(FeatureModel &FM1, FeatureModel &FM2, bool Strict) {
   return false;
 }
 
+std::optional<Relationship *> getFeatureRelationship(Feature &F) {
+  if (!F.getChildren<Relationship>(1).empty()) {
+    auto *C = *(F.children().begin());
+    if (auto *R = llvm::dyn_cast<Relationship>(C)) {
+      return R;
+    }
+  }
+  return std::nullopt;
+}
+
 } // namespace vara::feature

lib/Feature/FeatureModelWriter.cpp

@@ -27,6 +27,7 @@ int FeatureModelXmlWriter::writeFeatureModel(std::string Path) {
   std::unique_ptr<xmlTextWriter, void (*)(xmlTextWriterPtr)> Writer(
       xmlNewTextWriterFilename(Path.data(), 0), &xmlFreeTextWriter);
   if (!Writer) {
+    llvm::errs() << "Could not create filewriter for " << Path << '\n';
     return false;
   }
 

unittests/Feature/Feature.cpp

@@ -95,4 +95,20 @@ TEST(Feature, locationUpdate) {
   EXPECT_EQ(*TestLCO.getLocationsBegin(), Fsr2);
 }
 
+TEST(Feature, getChildren) {
+  FeatureModelBuilder B;
+  B.makeFeature<NumericFeature>("a", NumericFeature::ValueListType{1, 2, 3});
+  B.addEdge("a", "aa")->makeFeature<BinaryFeature>("aa");
+  B.addEdge("a", "ab")->makeFeature<BinaryFeature>("ab");
+
+  B.emplaceRelationship(Relationship::RelationshipKind::RK_ALTERNATIVE, "a");
+  auto FM = B.buildFeatureModel();
+  ASSERT_TRUE(FM);
+
+  EXPECT_EQ(FM->getFeature("a")->getChildren<Feature>(0).size(), 0);
+  EXPECT_EQ(FM->getFeature("a")->getChildren<Feature>(1).size(), 0);
+  EXPECT_EQ(FM->getFeature("a")->getChildren<Feature>(2).size(), 2);
+  EXPECT_EQ(FM->getFeature("a")->getChildren<Feature>(3).size(), 2);
+}
+
 } // namespace vara::feature

unittests/Feature/FeatureModel.cpp

@@ -220,6 +220,34 @@ TEST_F(FeatureModelTest, dfs) {
   }
 }
 
+//===----------------------------------------------------------------------===//
+//                    FeatureModelGetChildren Tests
+//===----------------------------------------------------------------------===//
+
+class FeatureModelGetChildrenTest : public ::testing::Test {
+protected:
+  void SetUp() override {
+    FeatureModelBuilder B;
+    B.makeFeature<BinaryFeature>("a");
+    B.addEdge("a", "aa")->makeFeature<BinaryFeature>("aa");
+    B.addEdge("a", "ab")->makeFeature<BinaryFeature>("ab");
+    B.emplaceRelationship(Relationship::RelationshipKind::RK_OR, "ab");
+    B.addEdge("ab", "aba")->makeFeature<BinaryFeature>("aba");
+    B.addEdge("ab", "abb")->makeFeature<BinaryFeature>("abb");
+    B.makeFeature<BinaryFeature>("b");
+    B.emplaceRelationship(Relationship::RelationshipKind::RK_ALTERNATIVE, "b");
+    B.addEdge("b", "ba")->makeFeature<BinaryFeature>("ba");
+    B.addEdge("b", "bb")->makeFeature<BinaryFeature>("bb");
+    B.makeFeature<BinaryFeature>("c");
+    B.addEdge("c", "ca")->makeFeature<NumericFeature>("ca", std::pair(1, 5));
+    B.addEdge("c", "cb")->makeFeature<NumericFeature>("cb", std::pair(1, 5));
+    FM = B.buildFeatureModel();
+    ASSERT_TRUE(FM);
+  }
+
+  std::unique_ptr<FeatureModel> FM;
+};
+
 //===----------------------------------------------------------------------===//
 //                    FeatureModelConsistencyChecker Tests
 //===----------------------------------------------------------------------===//