From 2a30dccbd5dcba3e9e16d70b7a2f695fc1227a14 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pascal=20Dupr=C3=A9?= Date: Mon, 24 May 2021 03:20:32 +0200 Subject: [PATCH 01/17] Fixes to merging algorithm to compensate xml formats limitations Also tests --- .../vara/Feature/FeatureModelTransaction.h | 8 +- lib/Feature/FeatureModelTransaction.cpp | 47 +++- unittests/Feature/FeatureModel.cpp | 36 +++ unittests/Feature/FeatureModelTransaction.cpp | 224 ++++++++++++++++++ 4 files changed, 308 insertions(+), 7 deletions(-) diff --git a/include/vara/Feature/FeatureModelTransaction.h b/include/vara/Feature/FeatureModelTransaction.h index 886147186..854e3a2a1 100644 --- a/include/vara/Feature/FeatureModelTransaction.h +++ b/include/vara/Feature/FeatureModelTransaction.h @@ -304,8 +304,12 @@ class AddFeatureToModel : public FeatureModelModification { return nullptr; } if (Parent) { - setParent(*InsertedFeature, *Parent); - addEdge(*Parent, *InsertedFeature); + FeatureTreeNode *ParentNode = Parent; + if (!Parent->getChildren().empty()) { + ParentNode = *(Parent->getChildren().begin()); + } + setParent(*InsertedFeature, *ParentNode); + addEdge(*ParentNode, *InsertedFeature); } else if (FM.getRoot()) { setParent(*InsertedFeature, *FM.getRoot()); addEdge(*FM.getRoot(), *InsertedFeature); diff --git a/lib/Feature/FeatureModelTransaction.cpp b/lib/Feature/FeatureModelTransaction.cpp index b304afa86..5c7749cbf 100644 --- a/lib/Feature/FeatureModelTransaction.cpp +++ b/lib/Feature/FeatureModelTransaction.cpp @@ -7,10 +7,15 @@ namespace vara::feature { bool mergeSubtree(FeatureModelCopyTransaction &Trans, FeatureModel const &FM, Feature &F, bool Strict); +bool mergeSubtree(FeatureModelCopyTransaction &Trans, FeatureModel const &FM, + Relationship &R, bool Strict); + std::unique_ptr FeatureCopy(Feature &F); bool CompareProperties(const Feature &F1, const Feature &F2, bool Strict); +std::optional getFeatureRelationship(Feature &F); + void addFeature(FeatureModel &FM, std::unique_ptr NewFeature, Feature *Parent) { auto Trans = FeatureModelModifyTransaction::openTransaction(FM); @@ -73,7 +78,25 @@ 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 + if (FRelationship.value()->getKind() != + CMPRelationship.value()->getKind()) { + return false; + } + } else if (FRelationship && !CMPRelationship) { + if (CMP->getChildren().size() < 2) { + Trans.addRelationship(FRelationship.value()->getKind(), F.getName()); + } else { + 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 +112,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()); + } } - // copy children if missing for (Feature *Child : F.getChildren()) { if (!mergeSubtree(Trans, FM, *Child, Strict)) { return false; @@ -124,9 +151,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 +176,9 @@ 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.getParent()->getKind() != F2.getParent()->getKind()) { + // return false; + // } if (F1.getKind() == Feature::FeatureKind::FK_BINARY) { return true; } @@ -161,4 +190,12 @@ mergeFeatureModels(FeatureModel &FM1, FeatureModel &FM2, bool Strict) { return false; } +std::optional getFeatureRelationship(Feature &F) { + auto Relationships = F.getChildren(); + if (Relationships.empty()) { + return std::nullopt; + } + return *(Relationships.begin()); +} + } // namespace vara::feature diff --git a/unittests/Feature/FeatureModel.cpp b/unittests/Feature/FeatureModel.cpp index 5c9f1eadd..0f8ce5439 100644 --- a/unittests/Feature/FeatureModel.cpp +++ b/unittests/Feature/FeatureModel.cpp @@ -224,6 +224,42 @@ TEST_F(FeatureModelTest, dfs) { // FeatureModelConsistencyChecker Tests //===----------------------------------------------------------------------===// +class FeatureModelGetChildrenTest : public ::testing::Test { +protected: + void SetUp() override { + FeatureModelBuilder B; + B.makeFeature("a"); + B.addEdge("a", "aa")->makeFeature("aa"); + B.addEdge("a", "ab")->makeFeature("ab"); + B.emplaceRelationship(Relationship::RelationshipKind::RK_OR, "ab"); + B.addEdge("ab", "aba")->makeFeature("aba"); + B.addEdge("ab", "abb")->makeFeature("abb"); + B.makeFeature("b"); + B.emplaceRelationship(Relationship::RelationshipKind::RK_ALTERNATIVE, "b"); + B.addEdge("b", "ba")->makeFeature("ba"); + B.addEdge("b", "bb")->makeFeature("bb"); + B.makeFeature("c"); + B.addEdge("c", "ca")->makeFeature("ca", std::pair(1, 5)); + B.addEdge("c", "cb")->makeFeature("cb", std::pair(1, 5)); + FM = B.buildFeatureModel(); + ASSERT_TRUE(FM); + } + + std::unique_ptr FM; +}; + +TEST_F(FeatureModelGetChildrenTest, Root_getRelationships) { + EXPECT_EQ(FM->getRoot()->getChildren().size(), 0); +} + +TEST_F(FeatureModelGetChildrenTest, Root_getFeature) { + EXPECT_EQ(FM->getRoot()->getChildren().size(), 3); +} + +//===----------------------------------------------------------------------===// +// FeatureModelConsistencyChecker Tests +//===----------------------------------------------------------------------===// + class FeatureModelConsistencyCheckerTest : public ::testing::Test, protected detail::FeatureModelModification { diff --git a/unittests/Feature/FeatureModelTransaction.cpp b/unittests/Feature/FeatureModelTransaction.cpp index ee03b15b9..6249be078 100644 --- a/unittests/Feature/FeatureModelTransaction.cpp +++ b/unittests/Feature/FeatureModelTransaction.cpp @@ -394,6 +394,230 @@ TEST_F(FeatureModelMergeTransactionTest, AcceptNonStrictDifferenceKind) { EXPECT_EQ(F1->getKind(), Feature::FeatureKind::FK_BINARY); } +//===----------------------------------------------------------------------===// +// FeatureModelMergeRelationshipsTransaction Tests +//===----------------------------------------------------------------------===// + +class FeatureModelMergeRelationshipsTransactionTest : public ::testing::Test { +protected: + void SetUp() override { + FeatureModelBuilder B; + B.makeFeature("a", true); + B.emplaceRelationship(Relationship::RelationshipKind::RK_ALTERNATIVE, "a"); + B.makeFeature("aa", false); + B.makeFeature("ab", false); + B.addEdge("a", "aa"); + B.addEdge("a", "ab"); + FM1 = B.buildFeatureModel(); + ASSERT_TRUE(FM1); + ASSERT_FALSE(FM1->getFeature("a")->getChildren().empty()); + } + + std::unique_ptr FM1; +}; + +TEST_F(FeatureModelMergeRelationshipsTransactionTest, Idempotence) { + size_t FMSizeBefore = FM1->size(); + auto FMMerged = mergeFeatureModels(*FM1, *FM1); + ASSERT_TRUE(FMMerged); + + // Relationship should be in merge result + EXPECT_EQ(FMMerged->size(), FMSizeBefore); + EXPECT_TRUE(FMMerged->getFeature("root")); + ASSERT_TRUE(FMMerged->getFeature("a")); + EXPECT_EQ(FMMerged->getFeature("a")->getChildren().size(), 1); + ASSERT_TRUE(FMMerged->getFeature("aa")); + EXPECT_EQ(FMMerged->getFeature("aa")->getParent()->getKind(), + FeatureTreeNode::NodeKind::NK_RELATIONSHIP); + ASSERT_TRUE(FMMerged->getFeature("ab")); + EXPECT_EQ(FMMerged->getFeature("ab")->getParent()->getKind(), + FeatureTreeNode::NodeKind::NK_RELATIONSHIP); +} + +TEST_F(FeatureModelMergeRelationshipsTransactionTest, + RejectMissingRelationship) { + FeatureModelBuilder B; + B.makeFeature("a", true); + B.makeFeature("aa", false); + B.makeFeature("ab", false); + B.addEdge("a", "aa"); + std::unique_ptr FM2 = B.buildFeatureModel(); + ASSERT_TRUE(FM2); + + // Expect fail, relationships should match if both have more than one child + auto FMMerged = mergeFeatureModels(*FM1, *FM2); + EXPECT_FALSE(FMMerged); + + // Expect fail, also if direction is reversed + FMMerged = mergeFeatureModels(*FM2, *FM1); + EXPECT_FALSE(FMMerged); +} + +TEST_F(FeatureModelMergeRelationshipsTransactionTest, + CheckRelationshipsOfAdded) { + FeatureModelBuilder B; + std::unique_ptr FM2 = B.buildFeatureModel(); + ASSERT_TRUE(FM2); + + // Expect success, feature group of "a" should also be a group after merge. + auto FMMerged = mergeFeatureModels(*FM2, *FM1); + ASSERT_TRUE(FMMerged); + + ASSERT_TRUE(FMMerged->getFeature("a")); + ASSERT_TRUE(FMMerged->getFeature("aa")); + EXPECT_EQ(FMMerged->getFeature("aa")->getParent()->getKind(), + FeatureTreeNode::NodeKind::NK_RELATIONSHIP); + ASSERT_TRUE(FMMerged->getFeature("ab")); + EXPECT_EQ(FMMerged->getFeature("ab")->getParent()->getKind(), + FeatureTreeNode::NodeKind::NK_RELATIONSHIP); +} + +TEST_F(FeatureModelMergeRelationshipsTransactionTest, + AcceptMissingRelationshipOnLeaf) { + FeatureModelBuilder B; + B.makeFeature("a", true); + std::unique_ptr FM2 = B.buildFeatureModel(); + ASSERT_TRUE(FM2); + + // Expect success, feature should be merged even if relationship is only + // present in original. + auto FMMerged = mergeFeatureModels(*FM1, *FM2); + ASSERT_TRUE(FMMerged); + + ASSERT_TRUE(FMMerged->getFeature("a")); + ASSERT_TRUE(FMMerged->getFeature("aa")); + EXPECT_EQ(FMMerged->getFeature("aa")->getParent()->getKind(), + FeatureTreeNode::NodeKind::NK_RELATIONSHIP); + ASSERT_TRUE(FMMerged->getFeature("ab")); + EXPECT_EQ(FMMerged->getFeature("ab")->getParent()->getKind(), + FeatureTreeNode::NodeKind::NK_RELATIONSHIP); +} + +TEST_F(FeatureModelMergeRelationshipsTransactionTest, + AcceptMissingRelationshipOnLeafReversed) { + FeatureModelBuilder B; + B.makeFeature("a", true); + std::unique_ptr FM2 = B.buildFeatureModel(); + ASSERT_TRUE(FM2); + + // Expect success, relationship should be added to childless feature in FM2. + // Fixes XML format shortcomings (groups cannot be specified for 0 or 1 child) + auto FMMerged = mergeFeatureModels(*FM2, *FM1); + ASSERT_TRUE(FMMerged); + + ASSERT_TRUE(FMMerged->getFeature("a")); + ASSERT_TRUE(FMMerged->getFeature("aa")); + EXPECT_EQ(FMMerged->getFeature("aa")->getParent()->getKind(), + FeatureTreeNode::NodeKind::NK_RELATIONSHIP); + ASSERT_TRUE(FMMerged->getFeature("ab")); + EXPECT_EQ(FMMerged->getFeature("ab")->getParent()->getKind(), + FeatureTreeNode::NodeKind::NK_RELATIONSHIP); +} + +TEST_F(FeatureModelMergeRelationshipsTransactionTest, + AcceptMissingRelationshipForSingleChild) { + FeatureModelBuilder B; + B.makeFeature("a", true); + B.makeFeature("ab", false); + B.addEdge("a", "ab"); + std::unique_ptr FM2 = B.buildFeatureModel(); + ASSERT_TRUE(FM2); + + // Expect success, relationship should be added to childless feature in FM2. + // Fixes XML format shortcomings (groups cannot be specified for 0 or 1 child) + auto FMMerged = mergeFeatureModels(*FM1, *FM2); + ASSERT_TRUE(FMMerged); + + ASSERT_TRUE(FMMerged->getFeature("a")); + ASSERT_TRUE(FMMerged->getFeature("aa")); + EXPECT_EQ(FMMerged->getFeature("aa")->getParent()->getKind(), + FeatureTreeNode::NodeKind::NK_RELATIONSHIP); + ASSERT_TRUE(FMMerged->getFeature("ab")); + EXPECT_EQ(FMMerged->getFeature("ab")->getParent()->getKind(), + FeatureTreeNode::NodeKind::NK_RELATIONSHIP); +} + +TEST_F(FeatureModelMergeRelationshipsTransactionTest, + AcceptMissingRelationshipForSingleChildReversed) { + FeatureModelBuilder B; + B.makeFeature("a", true); + B.makeFeature("ab", false); + B.addEdge("a", "ab"); + std::unique_ptr FM2 = B.buildFeatureModel(); + ASSERT_TRUE(FM2); + + // Expect success, relationship should be added to childless feature in FM2. + // Fixes XML format shortcomings (groups cannot be specified for 0 or 1 child) + auto FMMerged = mergeFeatureModels(*FM2, *FM1); + ASSERT_TRUE(FMMerged); + + ASSERT_TRUE(FMMerged->getFeature("a")); + ASSERT_TRUE(FMMerged->getFeature("aa")); + EXPECT_EQ(FMMerged->getFeature("aa")->getParent()->getKind(), + FeatureTreeNode::NodeKind::NK_RELATIONSHIP); + ASSERT_TRUE(FMMerged->getFeature("ab")); + EXPECT_EQ(FMMerged->getFeature("ab")->getParent()->getKind(), + FeatureTreeNode::NodeKind::NK_RELATIONSHIP); +} + +TEST_F(FeatureModelMergeRelationshipsTransactionTest, + MissingRelationshipNonStrict) { + size_t FMSizeBefore = FM1->size(); + FeatureModelBuilder B; + B.makeFeature("a", true); + B.makeFeature("aa", false); + B.makeFeature("ab", false); + B.addEdge("a", "aa"); + B.addEdge("a", "ab"); + std::unique_ptr FM2 = B.buildFeatureModel(); + ASSERT_TRUE(FM2); + ASSERT_EQ(FMSizeBefore, FM2->size()); + + // Expect success, relationships of first Model should be used + auto FMMerged = mergeFeatureModels(*FM1, *FM2, false); + ASSERT_TRUE(FMMerged); + + EXPECT_EQ(FMMerged->size(), FMSizeBefore); + ASSERT_TRUE(FMMerged->getFeature("root")); + ASSERT_TRUE(FMMerged->getFeature("a")); + EXPECT_EQ(FMMerged->getFeature("a")->getChildren().size(), 1); + ASSERT_TRUE(FMMerged->getFeature("aa")); + EXPECT_EQ(FMMerged->getFeature("aa")->getParent()->getKind(), + FeatureTreeNode::NodeKind::NK_RELATIONSHIP); + ASSERT_TRUE(FMMerged->getFeature("ab")); + EXPECT_EQ(FMMerged->getFeature("ab")->getParent()->getKind(), + FeatureTreeNode::NodeKind::NK_RELATIONSHIP); +} + +TEST_F(FeatureModelMergeRelationshipsTransactionTest, + MissingRelationshipNonStrictReversed) { + size_t FMSizeBefore = FM1->size(); + FeatureModelBuilder B; + B.makeFeature("a", true); + B.makeFeature("aa", false); + B.makeFeature("ab", false); + B.addEdge("a", "aa"); + B.addEdge("a", "ab"); + std::unique_ptr FM2 = B.buildFeatureModel(); + ASSERT_TRUE(FM2); + ASSERT_EQ(FMSizeBefore, FM2->size()); + + // Expect success, relationships of first Model should be used + auto FMMerged = mergeFeatureModels(*FM2, *FM1, false); + ASSERT_TRUE(FMMerged); + + EXPECT_EQ(FMMerged->size(), FMSizeBefore); + ASSERT_TRUE(FMMerged->getFeature("root")); + ASSERT_TRUE(FMMerged->getFeature("a")); + EXPECT_EQ(FMMerged->getFeature("a")->getChildren().size(), 1); + ASSERT_TRUE(FMMerged->getFeature("aa")); + EXPECT_EQ(FMMerged->getFeature("aa")->getParent()->getKind(), + FeatureTreeNode::NodeKind::NK_RELATIONSHIP); + ASSERT_TRUE(FMMerged->getFeature("ab")); + EXPECT_EQ(FMMerged->getFeature("ab")->getParent()->getKind(), + FeatureTreeNode::NodeKind::NK_RELATIONSHIP); +} + //===----------------------------------------------------------------------===// // FeatureModelRelationshipTransaction Tests //===----------------------------------------------------------------------===// From d3edd74e89c963c8d721b917ebefeac6b29ea68a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pascal=20Dupr=C3=A9?= Date: Mon, 24 May 2021 03:45:16 +0200 Subject: [PATCH 02/17] Fix --- lib/Feature/FeatureModelTransaction.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/Feature/FeatureModelTransaction.cpp b/lib/Feature/FeatureModelTransaction.cpp index 5c7749cbf..cb27e380a 100644 --- a/lib/Feature/FeatureModelTransaction.cpp +++ b/lib/Feature/FeatureModelTransaction.cpp @@ -89,7 +89,8 @@ mergeFeatureModels(FeatureModel &FM1, FeatureModel &FM2, bool Strict) { } } else if (FRelationship && !CMPRelationship) { if (CMP->getChildren().size() < 2) { - Trans.addRelationship(FRelationship.value()->getKind(), F.getName()); + Trans.addRelationship(FRelationship.value()->getKind(), + F.getName().str()); } else { return false; } From b4ac25731ea75a50eb7ad35eba04fa133beaeaa5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pascal=20Dupr=C3=A9?= Date: Mon, 24 May 2021 03:55:24 +0200 Subject: [PATCH 03/17] Fix --- lib/Feature/FeatureModelTransaction.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Feature/FeatureModelTransaction.cpp b/lib/Feature/FeatureModelTransaction.cpp index cb27e380a..e9ba01c65 100644 --- a/lib/Feature/FeatureModelTransaction.cpp +++ b/lib/Feature/FeatureModelTransaction.cpp @@ -116,7 +116,7 @@ mergeFeatureModels(FeatureModel &FM1, FeatureModel &FM2, bool Strict) { // add relationship to new feature auto Relationship = getFeatureRelationship(F); if (Relationship.has_value()) { - Trans.addRelationship(Relationship.value()->getKind(), F.getName()); + Trans.addRelationship(Relationship.value()->getKind(), F.getName().str()); } } From 99f61306ecc034ce3a233a0cd9f56af670543c46 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pascal=20Dupr=C3=A9?= Date: Mon, 24 May 2021 16:00:03 +0200 Subject: [PATCH 04/17] Workaround for getChildren, relationship copy for non strict fixed --- lib/Feature/FeatureModelTransaction.cpp | 19 +++++++++++-------- unittests/Feature/FeatureModelTransaction.cpp | 8 +++----- 2 files changed, 14 insertions(+), 13 deletions(-) diff --git a/lib/Feature/FeatureModelTransaction.cpp b/lib/Feature/FeatureModelTransaction.cpp index e9ba01c65..aac9f517d 100644 --- a/lib/Feature/FeatureModelTransaction.cpp +++ b/lib/Feature/FeatureModelTransaction.cpp @@ -82,16 +82,17 @@ mergeFeatureModels(FeatureModel &FM1, FeatureModel &FM2, bool Strict) { auto FRelationship = getFeatureRelationship(F); auto CMPRelationship = getFeatureRelationship(*CMP); if (FRelationship && CMPRelationship) { - // Relationships must match - if (FRelationship.value()->getKind() != - CMPRelationship.value()->getKind()) { + // 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().size() < 2) { Trans.addRelationship(FRelationship.value()->getKind(), F.getName().str()); - } else { + } else if (Strict) { return false; } } @@ -192,11 +193,13 @@ mergeFeatureModels(FeatureModel &FM1, FeatureModel &FM2, bool Strict) { } std::optional getFeatureRelationship(Feature &F) { - auto Relationships = F.getChildren(); - if (Relationships.empty()) { - return std::nullopt; + if (!F.children().empty()) { + auto *C = *(F.children().begin()); + if (auto *R = llvm::dyn_cast(C)) { + return R; + } } - return *(Relationships.begin()); + return std::nullopt; } } // namespace vara::feature diff --git a/unittests/Feature/FeatureModelTransaction.cpp b/unittests/Feature/FeatureModelTransaction.cpp index 6249be078..3535cf5f3 100644 --- a/unittests/Feature/FeatureModelTransaction.cpp +++ b/unittests/Feature/FeatureModelTransaction.cpp @@ -609,13 +609,11 @@ TEST_F(FeatureModelMergeRelationshipsTransactionTest, EXPECT_EQ(FMMerged->size(), FMSizeBefore); ASSERT_TRUE(FMMerged->getFeature("root")); ASSERT_TRUE(FMMerged->getFeature("a")); - EXPECT_EQ(FMMerged->getFeature("a")->getChildren().size(), 1); + // first model, fm2 has no relationship and "a" has 2 children so relationship + // is not moved + EXPECT_EQ(FMMerged->getFeature("a")->getChildren().size(), 0); ASSERT_TRUE(FMMerged->getFeature("aa")); - EXPECT_EQ(FMMerged->getFeature("aa")->getParent()->getKind(), - FeatureTreeNode::NodeKind::NK_RELATIONSHIP); ASSERT_TRUE(FMMerged->getFeature("ab")); - EXPECT_EQ(FMMerged->getFeature("ab")->getParent()->getKind(), - FeatureTreeNode::NodeKind::NK_RELATIONSHIP); } //===----------------------------------------------------------------------===// From 5d6a33e51ee5a7438fe4430c12f4d86c62b25df8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pascal=20Dupr=C3=A9?= Date: Tue, 1 Jun 2021 02:44:04 +0200 Subject: [PATCH 05/17] Stuff --- include/vara/Feature/FeatureModel.h | 8 ++ .../vara/Feature/FeatureModelTransaction.h | 74 +++++++++++++++++-- include/vara/Feature/FeatureTreeNode.h | 22 ++++-- lib/Feature/FeatureModelTransaction.cpp | 2 +- lib/Feature/FeatureModelWriter.cpp | 3 +- unittests/Feature/Feature.cpp | 17 +++++ unittests/Feature/FeatureModel.cpp | 8 -- unittests/Feature/FeatureModelParser.cpp | 11 +++ unittests/Feature/FeatureModelTransaction.cpp | 34 +++++++++ unittests/Feature/Relationship.cpp | 15 ++++ unittests/resources/CMakeLists.txt | 1 + 11 files changed, 175 insertions(+), 20 deletions(-) diff --git a/include/vara/Feature/FeatureModel.h b/include/vara/Feature/FeatureModel.h index fd98d9f91..6a60d17d6 100644 --- a/include/vara/Feature/FeatureModel.h +++ b/include/vara/Feature/FeatureModel.h @@ -286,6 +286,14 @@ class FeatureModel { return Constraints.back().get(); } + void removeConstraint(Constraint *C) { + Constraints.erase( + std::find_if(Constraints.begin(), Constraints.end(), + [C](const std::unique_ptr &UniC) { + return UniC.get() == C; + })); + } + /// Delete a \a Feature. void removeFeature(Feature &Feature); diff --git a/include/vara/Feature/FeatureModelTransaction.h b/include/vara/Feature/FeatureModelTransaction.h index 854e3a2a1..97466f07c 100644 --- a/include/vara/Feature/FeatureModelTransaction.h +++ b/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)); } @@ -305,8 +317,8 @@ class AddFeatureToModel : public FeatureModelModification { } if (Parent) { FeatureTreeNode *ParentNode = Parent; - if (!Parent->getChildren().empty()) { - ParentNode = *(Parent->getChildren().begin()); + if (!Parent->getChildren(1).empty()) { + ParentNode = *(Parent->getChildren(1).begin()); } setParent(*InsertedFeature, *ParentNode); addEdge(*ParentNode, *InsertedFeature); @@ -326,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 //===----------------------------------------------------------------------===// @@ -369,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); } @@ -805,14 +859,19 @@ class FeatureModelCopyTransactionBase { } Constraint *addConstraintImpl(std::unique_ptr NewConstraint) { - if (!FM) { - return nullptr; - } + assert(FM && "FeatureModel is null."); return FeatureModelModification::make_modification( std::move(NewConstraint))(*FM); } + void removeConstraintImpl(Constraint &RemoveConstraint) { + assert(FM && "FeatureModel is null."); + + FeatureModelModification::make_modification( + RemoveConstraint)(*FM); + } + void setNameImpl(std::string Name) { assert(FM && "FeatureModel is null."); @@ -921,6 +980,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."); diff --git a/include/vara/Feature/FeatureTreeNode.h b/include/vara/Feature/FeatureTreeNode.h index e86da483b..931ed7f61 100644 --- a/include/vara/Feature/FeatureTreeNode.h +++ b/include/vara/Feature/FeatureTreeNode.h @@ -76,10 +76,19 @@ class FeatureTreeNode : private llvm::DGNode { return std::find(begin(), end(), &N) != end(); } - /// Search all children of given type in tree structure. - template llvm::SmallSet getChildren() { + /// Search first (possibly transitive) children of given type in tree + /// structure. + /// + /// \tparam[in] T Type of expected nodes. + /// + /// \param[in] Depth Maximal recursion depth to which nodes, which does not + /// match T are expanded to their children. A depth of one means, only + /// children of this node are considered and disables expansions. + /// + /// \return Set of children nodes. + template llvm::SmallSet getChildren(int Depth = -1) { llvm::SmallSet FS; - getChildrenImpl(this, &FS); + getChildrenImpl(this, &FS, Depth); return FS; } @@ -107,12 +116,15 @@ class FeatureTreeNode : private llvm::DGNode { template static void getChildrenImpl(FeatureTreeNode *N, - llvm::SmallPtrSetImpl *FS) { + llvm::SmallPtrSetImpl *FS, int Depth) { + if (Depth == 0) { + return; + } for (auto *C : N->children()) { if (auto *F = llvm::dyn_cast(C); F) { FS->insert(F); } else { - getChildrenImpl(C, FS); + getChildrenImpl(C, FS, Depth - 1); } } } diff --git a/lib/Feature/FeatureModelTransaction.cpp b/lib/Feature/FeatureModelTransaction.cpp index aac9f517d..39ae8b557 100644 --- a/lib/Feature/FeatureModelTransaction.cpp +++ b/lib/Feature/FeatureModelTransaction.cpp @@ -193,7 +193,7 @@ mergeFeatureModels(FeatureModel &FM1, FeatureModel &FM2, bool Strict) { } std::optional getFeatureRelationship(Feature &F) { - if (!F.children().empty()) { + if (!F.getChildren(1).empty()) { auto *C = *(F.children().begin()); if (auto *R = llvm::dyn_cast(C)) { return R; diff --git a/lib/Feature/FeatureModelWriter.cpp b/lib/Feature/FeatureModelWriter.cpp index 1ea792c1c..1cad1b14a 100644 --- a/lib/Feature/FeatureModelWriter.cpp +++ b/lib/Feature/FeatureModelWriter.cpp @@ -25,8 +25,9 @@ static constexpr char ENCODING[] = "UTF-8"; int FeatureModelXmlWriter::writeFeatureModel(std::string Path) { int RC; std::unique_ptr Writer( - xmlNewTextWriterFilename(Path.data(), 0), &xmlFreeTextWriter); + xmlNewTextWriterFilename(Path.c_str(), 0), &xmlFreeTextWriter); if (!Writer) { + llvm::errs() << "Could not create filewriter for " << Path << '\n'; return false; } diff --git a/unittests/Feature/Feature.cpp b/unittests/Feature/Feature.cpp index 33218fd60..c777ecf1b 100644 --- a/unittests/Feature/Feature.cpp +++ b/unittests/Feature/Feature.cpp @@ -1,4 +1,5 @@ #include "vara/Feature/FeatureModel.h" +#include "vara/Feature/FeatureModelBuilder.h" #include "gtest/gtest.h" @@ -94,4 +95,20 @@ TEST(Feature, locationUpdate) { EXPECT_EQ(*TestLCO.getLocationsBegin(), Fsr2); } +TEST(Feature, getChildren) { + FeatureModelBuilder B; + B.makeFeature("a", std::vector{1, 2, 3}); + B.addEdge("a", "aa")->makeFeature("aa"); + B.addEdge("a", "ab")->makeFeature("ab"); + + B.emplaceRelationship(Relationship::RelationshipKind::RK_ALTERNATIVE, "a"); + auto FM = B.buildFeatureModel(); + ASSERT_TRUE(FM); + + EXPECT_EQ(FM->getFeature("a")->getChildren(0).size(), 0); + EXPECT_EQ(FM->getFeature("a")->getChildren(1).size(), 0); + EXPECT_EQ(FM->getFeature("a")->getChildren(2).size(), 2); + EXPECT_EQ(FM->getFeature("a")->getChildren(3).size(), 2); +} + } // namespace vara::feature diff --git a/unittests/Feature/FeatureModel.cpp b/unittests/Feature/FeatureModel.cpp index 0f8ce5439..a0760ad07 100644 --- a/unittests/Feature/FeatureModel.cpp +++ b/unittests/Feature/FeatureModel.cpp @@ -248,14 +248,6 @@ class FeatureModelGetChildrenTest : public ::testing::Test { std::unique_ptr FM; }; -TEST_F(FeatureModelGetChildrenTest, Root_getRelationships) { - EXPECT_EQ(FM->getRoot()->getChildren().size(), 0); -} - -TEST_F(FeatureModelGetChildrenTest, Root_getFeature) { - EXPECT_EQ(FM->getRoot()->getChildren().size(), 3); -} - //===----------------------------------------------------------------------===// // FeatureModelConsistencyChecker Tests //===----------------------------------------------------------------------===// diff --git a/unittests/Feature/FeatureModelParser.cpp b/unittests/Feature/FeatureModelParser.cpp index cebcd2fa2..87f7f4f1f 100644 --- a/unittests/Feature/FeatureModelParser.cpp +++ b/unittests/Feature/FeatureModelParser.cpp @@ -18,6 +18,17 @@ TEST(FeatureModelParser, onlyChildren) { EXPECT_TRUE(P.buildFeatureModel()); } +TEST(FeatureModelParser, test) { + auto FS = llvm::MemoryBuffer::getFileAsStream( + getTestResource("FeatureModel_v0.600.xml")); + assert(FS); + + auto P = FeatureModelXmlParser(FS.get()->getBuffer().str()); + + EXPECT_TRUE(P.verifyFeatureModel()); + EXPECT_TRUE(P.buildFeatureModel()); +} + TEST(FeatureModelParser, onlyParents) { auto FS = llvm::MemoryBuffer::getFileAsStream( getTestResource("test_only_parents.xml")); diff --git a/unittests/Feature/FeatureModelTransaction.cpp b/unittests/Feature/FeatureModelTransaction.cpp index 3535cf5f3..16f714768 100644 --- a/unittests/Feature/FeatureModelTransaction.cpp +++ b/unittests/Feature/FeatureModelTransaction.cpp @@ -1,9 +1,12 @@ #include "vara/Feature/FeatureModelTransaction.h" #include "vara/Feature/FeatureModelBuilder.h" +#include "UnittestHelper.h" #include "gtest/gtest.h" +#include #include +#include namespace vara::feature { namespace detail { @@ -616,6 +619,37 @@ TEST_F(FeatureModelMergeRelationshipsTransactionTest, ASSERT_TRUE(FMMerged->getFeature("ab")); } +TEST(SOMETHING, test) { + auto FS = llvm::MemoryBuffer::getFileAsStream( + getTestResource("FeatureModel_v0.600.xml")); + EXPECT_TRUE(FS && "Input file could not be read"); + auto FM = + FeatureModelXmlParser(FS.get()->getBuffer().str()).buildFeatureModel(); + + auto T = FeatureModelCopyTransaction::openTransaction(*FM); + detail::FeatureVariantTy V{FM->getFeature("level")}; + T.removeFeature(V); + V = FM->getFeature("maxWindowSize"); + T.removeFeature(V); + V = FM->getFeature("processorCount"); + T.removeFeature(V); + V = FM->getFeature("thresholdTestingDisabled"); + T.removeFeature(V); + V = FM->getFeature("compressionBzip2"); + T.removeFeature(V); + V = FM->getFeature("compressionGzip"); + T.removeFeature(V); + V = FM->getFeature("compressionDisabled"); + T.removeFeature(V); + V = FM->getFeature("compressionLzo"); + T.removeFeature(V); + V = FM->getFeature("compressionZpaq"); + T.removeFeature(V); + auto FM2 = T.commit(); + + auto FM3 = mergeFeatureModels(*FM2, *FM); +} + //===----------------------------------------------------------------------===// // FeatureModelRelationshipTransaction Tests //===----------------------------------------------------------------------===// diff --git a/unittests/Feature/Relationship.cpp b/unittests/Feature/Relationship.cpp index 0900cdd9a..435606e90 100644 --- a/unittests/Feature/Relationship.cpp +++ b/unittests/Feature/Relationship.cpp @@ -79,4 +79,19 @@ TEST(Relationship, outOfOrder) { } } +TEST(Relationship, getChildren) { + FeatureModelBuilder B; + B.makeFeature("a", std::vector{1, 2, 3}); + B.addEdge("a", "aa")->makeFeature("aa"); + B.addEdge("a", "ab")->makeFeature("ab"); + + B.emplaceRelationship(Relationship::RelationshipKind::RK_ALTERNATIVE, "a"); + auto FM = B.buildFeatureModel(); + ASSERT_TRUE(FM); + + EXPECT_EQ(FM->getFeature("a")->getChildren(0).size(), 0); + EXPECT_EQ(FM->getFeature("a")->getChildren(1).size(), 1); + EXPECT_EQ(FM->getFeature("a")->getChildren(2).size(), 1); +} + } // namespace vara::feature diff --git a/unittests/resources/CMakeLists.txt b/unittests/resources/CMakeLists.txt index ba56e5bf5..369d3bf4a 100644 --- a/unittests/resources/CMakeLists.txt +++ b/unittests/resources/CMakeLists.txt @@ -13,6 +13,7 @@ set(FEATURE_LIB_TEST_FILES test_only_children.xml test_only_parents.xml test_out_of_order.xml + FeatureModel_v0.600.xml sxfm/sxfm_example.sxfm sxfm/test.sxfm sxfm/test_wrong_indentation.sxfm From c17f47346867ee27f7f6729f48dabe53d8d057fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pascal=20Dupr=C3=A9?= Date: Tue, 8 Jun 2021 14:33:57 +0200 Subject: [PATCH 06/17] Eval version --- unittests/resources/FeatureModel_v0.600.xml | 299 ++++++++++++++++++++ 1 file changed, 299 insertions(+) create mode 100644 unittests/resources/FeatureModel_v0.600.xml diff --git a/unittests/resources/FeatureModel_v0.600.xml b/unittests/resources/FeatureModel_v0.600.xml new file mode 100644 index 000000000..eff59606d --- /dev/null +++ b/unittests/resources/FeatureModel_v0.600.xml @@ -0,0 +1,299 @@ + + + + root + + + + + + False + + + + compression + + + + root + + + False + + + + compressionBzip2 + + + + compression + + + compressionGzip + compressionLzo + compressionLzma + compressionZpaq + compressionDisabled + + False + + + main.c + + 545 + 4 + + + 545 + 41 + + + + + + + compressionGzip + + + + compression + + + compressionBzip2 + compressionLzo + compressionLzma + compressionZpaq + compressionDisabled + + False + + + main.c + + 566 + 4 + + + 566 + 40 + + + + + + + compressionLzo + + + + compression + + + compressionBzip2 + compressionGzip + compressionLzma + compressionZpaq + compressionDisabled + + False + + + main.c + + 584 + 4 + + + 584 + 39 + + + + + + + compressionLzma + + + + compression + + + compressionBzip2 + compressionGzip + compressionLzo + compressionZpaq + compressionDisabled + + False + + + + compressionZpaq + + + + compression + + + compressionBzip2 + compressionGzip + compressionLzo + compressionLzma + compressionDisabled + + False + + + main.c + + 663 + 4 + + + 663 + 40 + + + + + + + compressionDisabled + + + + compression + + + compressionBzip2 + compressionGzip + compressionLzo + compressionLzma + compressionZpaq + + False + + + main.c + + 594 + 4 + + + 594 + 38 + + + + + + + thresholdTestingDisabled + + + + root + + + True + + + main.c + + 638 + 4 + + + 638 + 40 + + + + + + + + + + level + + + + root + + + 1 + 9 + level + 1 + + + main.c + + 587 + 4 + + + 587 + 45 + + + + + + + + maxWindowSize + + + + root + + + 1 + 8 + maxWindowSize * 2 + + + main.c + + 658 + 4 + + + 658 + 34 + + + + + + + processorCount + + + + root + + + 1 + 8 + processorCount * 2 + + + main.c + + 618 + 4 + + + 618 + 35 + + + + + + + + From 4c94e951994a6821f46c091fd93ba3ddf9aa22e7 Mon Sep 17 00:00:00 2001 From: Florian Sattler Date: Fri, 23 Jul 2021 13:22:05 +0200 Subject: [PATCH 07/17] Update include/vara/Feature/FeatureTreeNode.h Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> --- include/vara/Feature/FeatureTreeNode.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/include/vara/Feature/FeatureTreeNode.h b/include/vara/Feature/FeatureTreeNode.h index 931ed7f61..3b962c3c8 100644 --- a/include/vara/Feature/FeatureTreeNode.h +++ b/include/vara/Feature/FeatureTreeNode.h @@ -86,7 +86,8 @@ class FeatureTreeNode : private llvm::DGNode { /// children of this node are considered and disables expansions. /// /// \return Set of children nodes. - template llvm::SmallSet getChildren(int Depth = -1) { + template + llvm::SmallSet getChildren(int Depth = -1) { llvm::SmallSet FS; getChildrenImpl(this, &FS, Depth); return FS; From eb6aabfa4161b217867043ebe139c56a6eb6e4ec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pascal=20Dupr=C3=A9?= Date: Fri, 23 Jul 2021 17:56:00 +0200 Subject: [PATCH 08/17] Revert merge error --- include/vara/Feature/FeatureTreeNode.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/vara/Feature/FeatureTreeNode.h b/include/vara/Feature/FeatureTreeNode.h index 3b962c3c8..6109e00ce 100644 --- a/include/vara/Feature/FeatureTreeNode.h +++ b/include/vara/Feature/FeatureTreeNode.h @@ -86,7 +86,7 @@ class FeatureTreeNode : private llvm::DGNode { /// children of this node are considered and disables expansions. /// /// \return Set of children nodes. - template + template llvm::SmallSet getChildren(int Depth = -1) { llvm::SmallSet FS; getChildrenImpl(this, &FS, Depth); From 5975b528c11f122cda775f27a635fe8246bb9de6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pascal=20Dupr=C3=A9?= Date: Fri, 23 Jul 2021 18:16:25 +0200 Subject: [PATCH 09/17] Remove old testcase, reproduced in other tests --- unittests/Feature/FeatureModelParser.cpp | 22 -- unittests/Feature/FeatureModelTransaction.cpp | 31 -- unittests/resources/FeatureModel_v0.600.xml | 299 ------------------ 3 files changed, 352 deletions(-) delete mode 100644 unittests/resources/FeatureModel_v0.600.xml diff --git a/unittests/Feature/FeatureModelParser.cpp b/unittests/Feature/FeatureModelParser.cpp index 40d241f23..873b4ddd8 100644 --- a/unittests/Feature/FeatureModelParser.cpp +++ b/unittests/Feature/FeatureModelParser.cpp @@ -29,28 +29,6 @@ TEST(FeatureModelParser, onlyChildren) { EXPECT_TRUE(P.buildFeatureModel()); } -TEST(FeatureModelParser, test) { - auto FS = llvm::MemoryBuffer::getFileAsStream( - getTestResource("FeatureModel_v0.600.xml")); - assert(FS); - - auto P = FeatureModelXmlParser(FS.get()->getBuffer().str()); - - EXPECT_TRUE(P.verifyFeatureModel()); - EXPECT_TRUE(P.buildFeatureModel()); -} - -TEST(FeatureModelParser, onlyParents) { - auto FS = llvm::MemoryBuffer::getFileAsStream( - getTestResource("test_only_parents.xml")); - assert(FS); - - auto P = FeatureModelXmlParser(FS.get()->getBuffer().str()); - - EXPECT_TRUE(P.verifyFeatureModel()); - EXPECT_TRUE(P.buildFeatureModel()); -} - TEST(FeatureModelParser, outOfOrder) { auto FS = llvm::MemoryBuffer::getFileAsStream( getTestResource("test_out_of_order.xml")); diff --git a/unittests/Feature/FeatureModelTransaction.cpp b/unittests/Feature/FeatureModelTransaction.cpp index 16f714768..fd0a56ae3 100644 --- a/unittests/Feature/FeatureModelTransaction.cpp +++ b/unittests/Feature/FeatureModelTransaction.cpp @@ -619,37 +619,6 @@ TEST_F(FeatureModelMergeRelationshipsTransactionTest, ASSERT_TRUE(FMMerged->getFeature("ab")); } -TEST(SOMETHING, test) { - auto FS = llvm::MemoryBuffer::getFileAsStream( - getTestResource("FeatureModel_v0.600.xml")); - EXPECT_TRUE(FS && "Input file could not be read"); - auto FM = - FeatureModelXmlParser(FS.get()->getBuffer().str()).buildFeatureModel(); - - auto T = FeatureModelCopyTransaction::openTransaction(*FM); - detail::FeatureVariantTy V{FM->getFeature("level")}; - T.removeFeature(V); - V = FM->getFeature("maxWindowSize"); - T.removeFeature(V); - V = FM->getFeature("processorCount"); - T.removeFeature(V); - V = FM->getFeature("thresholdTestingDisabled"); - T.removeFeature(V); - V = FM->getFeature("compressionBzip2"); - T.removeFeature(V); - V = FM->getFeature("compressionGzip"); - T.removeFeature(V); - V = FM->getFeature("compressionDisabled"); - T.removeFeature(V); - V = FM->getFeature("compressionLzo"); - T.removeFeature(V); - V = FM->getFeature("compressionZpaq"); - T.removeFeature(V); - auto FM2 = T.commit(); - - auto FM3 = mergeFeatureModels(*FM2, *FM); -} - //===----------------------------------------------------------------------===// // FeatureModelRelationshipTransaction Tests //===----------------------------------------------------------------------===// diff --git a/unittests/resources/FeatureModel_v0.600.xml b/unittests/resources/FeatureModel_v0.600.xml deleted file mode 100644 index eff59606d..000000000 --- a/unittests/resources/FeatureModel_v0.600.xml +++ /dev/null @@ -1,299 +0,0 @@ - - - - root - - - - - - False - - - - compression - - - - root - - - False - - - - compressionBzip2 - - - - compression - - - compressionGzip - compressionLzo - compressionLzma - compressionZpaq - compressionDisabled - - False - - - main.c - - 545 - 4 - - - 545 - 41 - - - - - - - compressionGzip - - - - compression - - - compressionBzip2 - compressionLzo - compressionLzma - compressionZpaq - compressionDisabled - - False - - - main.c - - 566 - 4 - - - 566 - 40 - - - - - - - compressionLzo - - - - compression - - - compressionBzip2 - compressionGzip - compressionLzma - compressionZpaq - compressionDisabled - - False - - - main.c - - 584 - 4 - - - 584 - 39 - - - - - - - compressionLzma - - - - compression - - - compressionBzip2 - compressionGzip - compressionLzo - compressionZpaq - compressionDisabled - - False - - - - compressionZpaq - - - - compression - - - compressionBzip2 - compressionGzip - compressionLzo - compressionLzma - compressionDisabled - - False - - - main.c - - 663 - 4 - - - 663 - 40 - - - - - - - compressionDisabled - - - - compression - - - compressionBzip2 - compressionGzip - compressionLzo - compressionLzma - compressionZpaq - - False - - - main.c - - 594 - 4 - - - 594 - 38 - - - - - - - thresholdTestingDisabled - - - - root - - - True - - - main.c - - 638 - 4 - - - 638 - 40 - - - - - - - - - - level - - - - root - - - 1 - 9 - level + 1 - - - main.c - - 587 - 4 - - - 587 - 45 - - - - - - - - maxWindowSize - - - - root - - - 1 - 8 - maxWindowSize * 2 - - - main.c - - 658 - 4 - - - 658 - 34 - - - - - - - processorCount - - - - root - - - 1 - 8 - processorCount * 2 - - - main.c - - 618 - 4 - - - 618 - 35 - - - - - - - - From 1025d36bffbaa04e9eb6c056dfbde2a972d69917 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pascal=20Dupr=C3=A9?= Date: Fri, 30 Jul 2021 15:19:35 +0200 Subject: [PATCH 10/17] Feedback --- include/vara/Feature/FeatureModel.h | 1 + lib/Feature/FeatureModelTransaction.cpp | 6 ------ lib/Feature/FeatureModelWriter.cpp | 2 +- unittests/Feature/FeatureModel.cpp | 2 +- unittests/Feature/FeatureModelParser.cpp | 11 +++++++++++ 5 files changed, 14 insertions(+), 8 deletions(-) diff --git a/include/vara/Feature/FeatureModel.h b/include/vara/Feature/FeatureModel.h index 6a60d17d6..acd326e3b 100644 --- a/include/vara/Feature/FeatureModel.h +++ b/include/vara/Feature/FeatureModel.h @@ -287,6 +287,7 @@ class FeatureModel { } 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 &UniC) { diff --git a/lib/Feature/FeatureModelTransaction.cpp b/lib/Feature/FeatureModelTransaction.cpp index 39ae8b557..081deec40 100644 --- a/lib/Feature/FeatureModelTransaction.cpp +++ b/lib/Feature/FeatureModelTransaction.cpp @@ -7,9 +7,6 @@ namespace vara::feature { bool mergeSubtree(FeatureModelCopyTransaction &Trans, FeatureModel const &FM, Feature &F, bool Strict); -bool mergeSubtree(FeatureModelCopyTransaction &Trans, FeatureModel const &FM, - Relationship &R, bool Strict); - std::unique_ptr FeatureCopy(Feature &F); bool CompareProperties(const Feature &F1, const Feature &F2, bool Strict); @@ -178,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; } diff --git a/lib/Feature/FeatureModelWriter.cpp b/lib/Feature/FeatureModelWriter.cpp index 20fbe5899..c7ca8472e 100644 --- a/lib/Feature/FeatureModelWriter.cpp +++ b/lib/Feature/FeatureModelWriter.cpp @@ -25,7 +25,7 @@ static constexpr char ENCODING[] = "UTF-8"; int FeatureModelXmlWriter::writeFeatureModel(std::string Path) { int RC; std::unique_ptr Writer( - xmlNewTextWriterFilename(Path.c_str(), 0), &xmlFreeTextWriter); + xmlNewTextWriterFilename(Path.data(), 0), &xmlFreeTextWriter); if (!Writer) { llvm::errs() << "Could not create filewriter for " << Path << '\n'; return false; diff --git a/unittests/Feature/FeatureModel.cpp b/unittests/Feature/FeatureModel.cpp index a0760ad07..1f604b7fe 100644 --- a/unittests/Feature/FeatureModel.cpp +++ b/unittests/Feature/FeatureModel.cpp @@ -221,7 +221,7 @@ TEST_F(FeatureModelTest, dfs) { } //===----------------------------------------------------------------------===// -// FeatureModelConsistencyChecker Tests +// FeatureModelGetChildren Tests //===----------------------------------------------------------------------===// class FeatureModelGetChildrenTest : public ::testing::Test { diff --git a/unittests/Feature/FeatureModelParser.cpp b/unittests/Feature/FeatureModelParser.cpp index 873b4ddd8..8f76302c3 100644 --- a/unittests/Feature/FeatureModelParser.cpp +++ b/unittests/Feature/FeatureModelParser.cpp @@ -29,6 +29,17 @@ TEST(FeatureModelParser, onlyChildren) { EXPECT_TRUE(P.buildFeatureModel()); } +TEST(FeatureModelParser, onlyParents) { + auto FS = llvm::MemoryBuffer::getFileAsStream( + getTestResource("test_only_parents.xml")); + assert(FS); + + auto P = FeatureModelXmlParser(FS.get()->getBuffer().str()); + + EXPECT_TRUE(P.verifyFeatureModel()); + EXPECT_TRUE(P.buildFeatureModel()); +} + TEST(FeatureModelParser, outOfOrder) { auto FS = llvm::MemoryBuffer::getFileAsStream( getTestResource("test_out_of_order.xml")); From 25a26e6b70e4c8f47a4efd3f367aa357c73417eb Mon Sep 17 00:00:00 2001 From: padupr <42845342+padupr@users.noreply.github.com> Date: Fri, 30 Jul 2021 15:21:56 +0200 Subject: [PATCH 11/17] Update unittests/Feature/FeatureModelTransaction.cpp Co-authored-by: Florian Sattler --- unittests/Feature/FeatureModelTransaction.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/unittests/Feature/FeatureModelTransaction.cpp b/unittests/Feature/FeatureModelTransaction.cpp index fd0a56ae3..d7bab3943 100644 --- a/unittests/Feature/FeatureModelTransaction.cpp +++ b/unittests/Feature/FeatureModelTransaction.cpp @@ -1,12 +1,15 @@ #include "vara/Feature/FeatureModelTransaction.h" #include "vara/Feature/FeatureModelBuilder.h" +#include "vara/Feature/FeatureModelParser.h" #include "UnittestHelper.h" + #include "gtest/gtest.h" -#include +#include "llvm/Support/MemoryBuffer.h" + #include -#include + namespace vara::feature { namespace detail { From 08c1ae554fffb653769787d1aa3159f3c0bfab54 Mon Sep 17 00:00:00 2001 From: padupr <42845342+padupr@users.noreply.github.com> Date: Fri, 30 Jul 2021 15:40:11 +0200 Subject: [PATCH 12/17] Update unittests/Feature/FeatureModelTransaction.cpp Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> --- unittests/Feature/FeatureModelTransaction.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/unittests/Feature/FeatureModelTransaction.cpp b/unittests/Feature/FeatureModelTransaction.cpp index d7bab3943..c039d4511 100644 --- a/unittests/Feature/FeatureModelTransaction.cpp +++ b/unittests/Feature/FeatureModelTransaction.cpp @@ -10,7 +10,6 @@ #include - namespace vara::feature { namespace detail { From c4226e5ab68a4bf36bcc2edd48478d774cec210e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pascal=20Dupr=C3=A9?= Date: Sat, 28 Aug 2021 16:43:28 +0200 Subject: [PATCH 13/17] Feedback --- bindings/python/vara-feature/pybind_Feature.cpp | 10 ++++++++++ .../python/vara-feature/pybind_FeatureSourceRange.cpp | 6 +++--- include/vara/Feature/FeatureModelTransaction.h | 3 ++- 3 files changed, 15 insertions(+), 4 deletions(-) diff --git a/bindings/python/vara-feature/pybind_Feature.cpp b/bindings/python/vara-feature/pybind_Feature.cpp index d40a8c5e9..66f6094f0 100644 --- a/bindings/python/vara-feature/pybind_Feature.cpp +++ b/bindings/python/vara-feature/pybind_Feature.cpp @@ -115,10 +115,20 @@ void init_feature_module_root_feature(py::module &M) { R"pbdoc(Returns the string representation of a RootFeature.)pbdoc"); } +void init_feature_module_relationship(py::module &M) { + py::class_(M, "Relationship") + .def("__hash__", &vf::Feature::hash) + .def(py::init) + py::enum_(Loc, "Category") + .value("necessary", vf::FeatureSourceRange::Category::necessary) + .value("inessential", vf::FeatureSourceRange::Category::inessential); +} + void init_feature_module(py::module &M) { init_feature_module_feature_tree_node(M); init_feature_module_feature(M); init_feature_module_binary_feature(M); init_feature_module_numeric_feature(M); init_feature_module_root_feature(M); + init_feature_module_relationship(M); } diff --git a/bindings/python/vara-feature/pybind_FeatureSourceRange.cpp b/bindings/python/vara-feature/pybind_FeatureSourceRange.cpp index 4893fbc63..ca102334d 100644 --- a/bindings/python/vara-feature/pybind_FeatureSourceRange.cpp +++ b/bindings/python/vara-feature/pybind_FeatureSourceRange.cpp @@ -70,9 +70,9 @@ void init_feature_location_module(py::module &M) { return Self == *Other; }, py::arg("Other").none(true)); - py::enum_(Loc, "Category") - .value("necessary", vf::FeatureSourceRange::Category::necessary) - .value("inessential", vf::FeatureSourceRange::Category::inessential); + py::enum_(Loc, "RelationshipKind") + .value("xor", vf::FeatureSourceRange::Category::necessary) + .value("or", vf::FeatureSourceRange::Category::inessential); py::class_(M, "LineColumnOffset") diff --git a/include/vara/Feature/FeatureModelTransaction.h b/include/vara/Feature/FeatureModelTransaction.h index 2645e69cb..9bf6e6008 100644 --- a/include/vara/Feature/FeatureModelTransaction.h +++ b/include/vara/Feature/FeatureModelTransaction.h @@ -405,7 +405,7 @@ class RemoveFeatureFromModel : public FeatureModelModification { } } else { if (!F->getChildren().empty()) { - // TODO (se-passau/VaRA#744): error, non recursive 0remove on non leaf + // TODO (se-passau/VaRA#744): error, non recursive remove on non leaf // feature return; } @@ -414,6 +414,7 @@ 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()) { From ee977c96ed3c23de6a9e8b83f455a61f9dab3e82 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pascal=20Dupr=C3=A9?= Date: Sat, 28 Aug 2021 19:05:07 +0200 Subject: [PATCH 14/17] Revert changes to bindings --- bindings/python/vara-feature/pybind_Feature.cpp | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/bindings/python/vara-feature/pybind_Feature.cpp b/bindings/python/vara-feature/pybind_Feature.cpp index 66f6094f0..d40a8c5e9 100644 --- a/bindings/python/vara-feature/pybind_Feature.cpp +++ b/bindings/python/vara-feature/pybind_Feature.cpp @@ -115,20 +115,10 @@ void init_feature_module_root_feature(py::module &M) { R"pbdoc(Returns the string representation of a RootFeature.)pbdoc"); } -void init_feature_module_relationship(py::module &M) { - py::class_(M, "Relationship") - .def("__hash__", &vf::Feature::hash) - .def(py::init) - py::enum_(Loc, "Category") - .value("necessary", vf::FeatureSourceRange::Category::necessary) - .value("inessential", vf::FeatureSourceRange::Category::inessential); -} - void init_feature_module(py::module &M) { init_feature_module_feature_tree_node(M); init_feature_module_feature(M); init_feature_module_binary_feature(M); init_feature_module_numeric_feature(M); init_feature_module_root_feature(M); - init_feature_module_relationship(M); } From f7e0e52e204c40ae01b59e4c320cfb3a2bf78c0e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pascal=20Dupr=C3=A9?= Date: Sat, 28 Aug 2021 19:20:30 +0200 Subject: [PATCH 15/17] Revert all changes to bindings --- bindings/python/vara-feature/pybind_FeatureSourceRange.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/bindings/python/vara-feature/pybind_FeatureSourceRange.cpp b/bindings/python/vara-feature/pybind_FeatureSourceRange.cpp index ca102334d..4893fbc63 100644 --- a/bindings/python/vara-feature/pybind_FeatureSourceRange.cpp +++ b/bindings/python/vara-feature/pybind_FeatureSourceRange.cpp @@ -70,9 +70,9 @@ void init_feature_location_module(py::module &M) { return Self == *Other; }, py::arg("Other").none(true)); - py::enum_(Loc, "RelationshipKind") - .value("xor", vf::FeatureSourceRange::Category::necessary) - .value("or", vf::FeatureSourceRange::Category::inessential); + py::enum_(Loc, "Category") + .value("necessary", vf::FeatureSourceRange::Category::necessary) + .value("inessential", vf::FeatureSourceRange::Category::inessential); py::class_(M, "LineColumnOffset") From e0f211b5c8689fc390269beeac5be851eea6a8d0 Mon Sep 17 00:00:00 2001 From: Florian Sattler Date: Mon, 17 Jan 2022 22:24:28 +0100 Subject: [PATCH 16/17] Fixes error --- include/vara/Feature/FeatureModelTransaction.h | 15 +++++++++++---- lib/Feature/FeatureModelTransaction.cpp | 2 +- unittests/Feature/Feature.cpp | 2 +- unittests/Feature/Relationship.cpp | 2 +- 4 files changed, 14 insertions(+), 7 deletions(-) diff --git a/include/vara/Feature/FeatureModelTransaction.h b/include/vara/Feature/FeatureModelTransaction.h index d217cc427..4d0a489b3 100644 --- a/include/vara/Feature/FeatureModelTransaction.h +++ b/include/vara/Feature/FeatureModelTransaction.h @@ -351,14 +351,21 @@ class RemoveConstraintFromModel : public FeatureModelModification { friend class RemoveFeatureFromModel; public: - void exec(FeatureModel &FM) override { (*this)(FM); } + Result exec(FeatureModel &FM) override { + if (auto E = (*this)(FM); !E) { + return E.getError(); + } + return Ok(); + } - void operator()(FeatureModel &FM) { + Result 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: @@ -961,7 +968,7 @@ class FeatureModelCopyTransactionBase { void removeConstraintImpl(Constraint &RemoveConstraint) { assert(FM && "FeatureModel is null."); - FeatureModelModification::make_modification( + FeatureModelModification::makeModification( RemoveConstraint)(*FM); } @@ -1087,7 +1094,7 @@ class FeatureModelModifyTransactionBase { } void removeConstraintImpl(Constraint &RemoveConstraint) { - Modifications.push_back(FeatureModelModification::make_unique_modification< + Modifications.push_back(FeatureModelModification::makeUniqueModification< RemoveConstraintFromModel>(RemoveConstraint)); } diff --git a/lib/Feature/FeatureModelTransaction.cpp b/lib/Feature/FeatureModelTransaction.cpp index 4e5727bc8..d5d0bc20c 100644 --- a/lib/Feature/FeatureModelTransaction.cpp +++ b/lib/Feature/FeatureModelTransaction.cpp @@ -176,7 +176,7 @@ mergeFeatureModels(FeatureModel &FM1, FeatureModel &FM2, bool Strict) { bool Strict) { // Is there a similar Feature in the original FM if (Feature *CMP = FM.getFeature(F.getName())) { - if (CompareProperties(*CMP, F, Strict)) { + if (compareProperties(*CMP, F, Strict)) { // similar feature, maybe add relationship auto FRelationship = getFeatureRelationship(F); auto CMPRelationship = getFeatureRelationship(*CMP); diff --git a/unittests/Feature/Feature.cpp b/unittests/Feature/Feature.cpp index c777ecf1b..1d9d57cd5 100644 --- a/unittests/Feature/Feature.cpp +++ b/unittests/Feature/Feature.cpp @@ -97,7 +97,7 @@ TEST(Feature, locationUpdate) { TEST(Feature, getChildren) { FeatureModelBuilder B; - B.makeFeature("a", std::vector{1, 2, 3}); + B.makeFeature("a", NumericFeature::ValueListType{1, 2, 3}); B.addEdge("a", "aa")->makeFeature("aa"); B.addEdge("a", "ab")->makeFeature("ab"); diff --git a/unittests/Feature/Relationship.cpp b/unittests/Feature/Relationship.cpp index 4341021ef..01e847133 100644 --- a/unittests/Feature/Relationship.cpp +++ b/unittests/Feature/Relationship.cpp @@ -80,7 +80,7 @@ TEST(Relationship, outOfOrder) { TEST(Relationship, getChildren) { FeatureModelBuilder B; - B.makeFeature("a", std::vector{1, 2, 3}); + B.makeFeature("a", NumericFeature::ValueListType{1, 2, 3}); B.addEdge("a", "aa")->makeFeature("aa"); B.addEdge("a", "ab")->makeFeature("ab"); From c4b01e56d84acdf3da3c19fb33ea78d7e1423a55 Mon Sep 17 00:00:00 2001 From: Florian Sattler Date: Fri, 4 Feb 2022 13:33:46 +0100 Subject: [PATCH 17/17] Adds error --- include/vara/Feature/Error.h | 5 ++++- include/vara/Feature/FeatureModelTransaction.h | 6 +++--- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/include/vara/Feature/Error.h b/include/vara/Feature/Error.h index 162c07de5..4cfd0aacf 100644 --- a/include/vara/Feature/Error.h +++ b/include/vara/Feature/Error.h @@ -22,7 +22,8 @@ enum FTErrorCode { MISSING_FEATURE, MISSING_MODEL, MISSING_PARENT, - NON_LEAF_NODE + NON_LEAF_NODE, + CONSTRAINT_MALFORMED }; } // namespace feature @@ -66,6 +67,8 @@ class Error { case vara::feature::NON_LEAF_NODE: OS << "Not a leaf node."; break; + case vara::feature::CONSTRAINT_MALFORMED: + OS << "Constraint is malformed."; } return OS; } diff --git a/include/vara/Feature/FeatureModelTransaction.h b/include/vara/Feature/FeatureModelTransaction.h index 4d0a489b3..78d655ce9 100644 --- a/include/vara/Feature/FeatureModelTransaction.h +++ b/include/vara/Feature/FeatureModelTransaction.h @@ -359,9 +359,9 @@ class RemoveConstraintFromModel : public FeatureModelModification { } Result operator()(FeatureModel &FM) { - // TODO: Any error handling needed here? - assert(RemoveConstraint.getParent() == nullptr && - "Subtree deletion is not supported"); + if (RemoveConstraint.getParent() == nullptr) { + return FTErrorCode::CONSTRAINT_MALFORMED; + } UncoupleVisitor UV; RemoveConstraint.accept(UV); removeConstraint(FM, &RemoveConstraint);