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

@@ -139,6 +139,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) {
@@ -253,6 +257,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));
   }
@@ -304,8 +316,12 @@ class AddFeatureToModel : public FeatureModelModification {
       return nullptr;
     }
     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);
@@ -322,6 +338,39 @@ class AddFeatureToModel : public FeatureModelModification {
   Feature *Parent;
 };
 
+//===----------------------------------------------------------------------===//
+//                       RemoveConstraintFromModel
+//===----------------------------------------------------------------------===//
+
+class RemoveConstraintFromModel : public FeatureModelModification {
+  friend class FeatureModelModification;
+  friend class RemoveFeatureFromModel;
+
+public:
+  void exec(FeatureModel &FM) override { (*this)(FM); }
+
+  void operator()(FeatureModel &FM) {
+    assert(RemoveConstraint.getParent() == nullptr &&
+           "Subtree deletion is not supported");
+    UncoupleVisitor UV;
+    RemoveConstraint.accept(UV);
+    removeConstraint(FM, &RemoveConstraint);
+  }
+
+private:
+  RemoveConstraintFromModel(Constraint &RemoveConstraint)
+      : RemoveConstraint(RemoveConstraint) {}
+
+  class UncoupleVisitor : public ConstraintVisitor {
+  public:
+    void visit(PrimaryFeatureConstraint *C) override {
+      removeConstraint(*C->getFeature(), C);
+    }
+  };
+
+  Constraint &RemoveConstraint;
+};
+
 //===----------------------------------------------------------------------===//
 //                          RemoveFeatureFromModel
 //===----------------------------------------------------------------------===//
@@ -365,6 +414,15 @@ class RemoveFeatureFromModel : public FeatureModelModification {
       }
     }
 
+    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);
   }
@@ -803,14 +861,19 @@ class FeatureModelCopyTransactionBase {
   }
 
   Constraint *addConstraintImpl(std::unique_ptr<Constraint> NewConstraint) {
-    if (!FM) {
-      return nullptr;
-    }
+    assert(FM && "FeatureModel is null.");
 
     return FeatureModelModification::make_modification<AddConstraintToModel>(
         std::move(NewConstraint))(*FM);
   }
 
+  void removeConstraintImpl(Constraint &RemoveConstraint) {
+    assert(FM && "FeatureModel is null.");
+
+    FeatureModelModification::make_modification<RemoveConstraintFromModel>(
+        RemoveConstraint)(*FM);
+  }
+
   void setNameImpl(std::string Name) {
     assert(FM && "FeatureModel is null.");
 
@@ -919,6 +982,11 @@ class FeatureModelModifyTransactionBase {
                             AddConstraintToModel>(std::move(NewConstraint)));
   }
 
+  void removeConstraintImpl(Constraint &RemoveConstraint) {
+    Modifications.push_back(FeatureModelModification::make_unique_modification<
+                            RemoveConstraintFromModel>(RemoveConstraint));
+  }
+
   void setNameImpl(std::string Name) {
     assert(FM && "FeatureModel is null.");
 

lib/Feature/FeatureModelTransaction.cpp

@@ -11,6 +11,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);
@@ -73,7 +75,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()) {
@@ -89,9 +111,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;
@@ -124,9 +150,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())) {
@@ -147,9 +175,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;
   }
@@ -161,4 +186,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", std::vector<int>{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
 //===----------------------------------------------------------------------===//