diff --git a/src/api/sorting/group_sort.cpp b/src/api/sorting/group_sort.cpp index 4bf019cd..f2d32cb5 100644 --- a/src/api/sorting/group_sort.cpp +++ b/src/api/sorting/group_sort.cpp @@ -34,39 +34,10 @@ #include "loot/exception/undefined_group_error.h" namespace loot { -class GroupSortingData { -public: - GroupSortingData() {} - GroupSortingData(std::string name) : name_(name) {} - - std::string GetName() const { return name_; } - - std::unordered_set GetMasterlistAfterGroups() const { - return masterlistAfterGroups_; - } - - std::unordered_set GetUserAfterGroups() const { - return userAfterGroups_; - } - - void SetMasterlistAfterGroups(std::unordered_set groups) { - masterlistAfterGroups_ = groups; - } - - void SetUserAfterGroups(std::unordered_set groups) { - userAfterGroups_ = groups; - } - -private: - std::string name_; - std::unordered_set masterlistAfterGroups_; - std::unordered_set userAfterGroups_; -}; - typedef boost::adjacency_list GroupGraph; typedef boost::graph_traits::vertex_descriptor vertex_t; @@ -79,7 +50,7 @@ public: visitedGroups_(visitedGroups) {} void tree_edge(edge_t edge, const GroupGraph& graph) { auto target = boost::target(edge, graph); - visitedGroups_.insert(graph[target].GetName()); + visitedGroups_.insert(graph[target]); } std::unordered_set get_visited_groups() const { @@ -90,6 +61,47 @@ private: std::unordered_set& visitedGroups_; }; +class CycleDetector : public boost::dfs_visitor<> { +public: + void tree_edge(edge_t edge, const GroupGraph& graph) { + auto source = boost::source(edge, graph); + + auto vertex = Vertex(graph[source], graph[edge]); + + // Check if the vertex already exists in the recorded trail. + auto it = find_if(begin(trail), end(trail), [&](const Vertex& v) { + return v.GetName() == graph[source]; + }); + + if (it != end(trail)) { + // Erase everything from this position onwards, as it doesn't + // contribute to a forward-cycle. + trail.erase(it, end(trail)); + } + + trail.push_back(vertex); + } + + void back_edge(edge_t edge, const GroupGraph& graph) { + auto source = boost::source(edge, graph); + auto target = boost::target(edge, graph); + + auto vertex = Vertex(graph[source], graph[edge]); + trail.push_back(vertex); + + auto it = find_if(begin(trail), end(trail), [&](const Vertex& v) { + return v.GetName() == graph[target]; + }); + + if (it != trail.end()) { + throw CyclicInteractionError(std::vector(it, trail.end())); + } + } + +private: + std::vector trail; +}; + std::string join(const std::unordered_set& set) { std::string output; for (const auto& element : set) { @@ -105,57 +117,52 @@ GroupGraph BuildGraph(const std::unordered_set& masterlistGroups, std::unordered_map groupVertices; for (const auto& group : masterlistGroups) { - auto groupSortingData = GroupSortingData(group.GetName()); - groupSortingData.SetMasterlistAfterGroups(group.GetAfterGroups()); - - auto vertex = boost::add_vertex(groupSortingData, graph); + auto vertex = boost::add_vertex(group.GetName(), graph); groupVertices.emplace(group.GetName(), vertex); } - for (const auto& group : userGroups) { - auto it = groupVertices.find(group.GetName()); - if (it != groupVertices.end()) { - graph[it->second].SetUserAfterGroups(group.GetAfterGroups()); - } else { - auto groupSortingData = GroupSortingData(group.GetName()); - groupSortingData.SetUserAfterGroups(group.GetAfterGroups()); - auto vertex = boost::add_vertex(groupSortingData, graph); + auto logger = getLogger(); + for (const auto& group : masterlistGroups) { + if (logger) { + logger->trace( + "Masterlist group \"{}\" directly loads after groups \"{}\"", + group.GetName(), + join(group.GetAfterGroups())); + } + + auto vertex = groupVertices.at(group.GetName()); + for (const auto& otherGroupName : group.GetAfterGroups()) { + auto otherVertex = groupVertices.find(otherGroupName); + if (otherVertex == groupVertices.end()) { + throw UndefinedGroupError(otherGroupName); + } + + boost::add_edge( + vertex, otherVertex->second, EdgeType::masterlistLoadAfter, graph); + } + } + + for (const auto& group : userGroups) { + if (groupVertices.find(group.GetName()) == groupVertices.end()) { + auto vertex = boost::add_vertex(group.GetName(), graph); groupVertices.emplace(group.GetName(), vertex); } } - auto logger = getLogger(); - for (const vertex_t& vertex : - boost::make_iterator_range(boost::vertices(graph))) { - auto group = graph[vertex]; - + for (const auto& group : userGroups) { if (logger) { - logger->trace( - "Group \"{}\" directly loads after masterlist groups \"{}\" and user " - "groups \"{}\"", - group.GetName(), - join(group.GetMasterlistAfterGroups()), - join(group.GetUserAfterGroups())); + logger->trace("Userlist group \"{}\" directly loads after groups \"{}\"", + group.GetName(), + join(group.GetAfterGroups())); } - for (const auto& otherGroupName : group.GetMasterlistAfterGroups()) { + auto vertex = groupVertices.at(group.GetName()); + for (const auto& otherGroupName : group.GetAfterGroups()) { auto otherVertex = groupVertices.find(otherGroupName); if (otherVertex == groupVertices.end()) { throw UndefinedGroupError(otherGroupName); } - auto vertex = groupVertices[group.GetName()]; - boost::add_edge( - vertex, otherVertex->second, EdgeType::masterlistLoadAfter, graph); - } - - for (const auto& otherGroupName : group.GetUserAfterGroups()) { - auto otherVertex = groupVertices.find(otherGroupName); - if (otherVertex == groupVertices.end()) { - throw UndefinedGroupError(otherGroupName); - } - - auto vertex = groupVertices[group.GetName()]; boost::add_edge( vertex, otherVertex->second, EdgeType::userLoadAfter, graph); } @@ -178,7 +185,7 @@ GetTransitiveAfterGroups(const std::unordered_set& masterlistGroups, if (logger) { logger->trace("Checking for cycles in the group graph"); } - boost::depth_first_search(graph, boost::visitor(CycleDetector())); + boost::depth_first_search(graph, boost::visitor(CycleDetector())); std::unordered_map> transitiveAfterGroups; @@ -193,11 +200,11 @@ GetTransitiveAfterGroups(const std::unordered_set& masterlistGroups, colorVec.begin(), boost::get(boost::vertex_index, graph), colorVec[0]); boost::depth_first_visit(graph, vertex, afterGroupsVisitor, colorMap); - transitiveAfterGroups[graph[vertex].GetName()] = visitedGroups; + transitiveAfterGroups[graph[vertex]] = visitedGroups; if (logger) { logger->trace("Group \"{}\" transitively loads after groups \"{}\"", - graph[vertex].GetName(), + graph[vertex], join(visitedGroups)); } } @@ -208,7 +215,7 @@ GetTransitiveAfterGroups(const std::unordered_set& masterlistGroups, vertex_t GetVertexByName(const GroupGraph& graph, const std::string& name) { for (const auto& vertex : boost::make_iterator_range(boost::vertices(graph))) { - if (graph[vertex].GetName() == name) { + if (graph[vertex] == name) { return vertex; } } @@ -221,7 +228,6 @@ vertex_t GetVertexByName(const GroupGraph& graph, const std::string& name) { throw std::invalid_argument("Can't find group with name \"" + name + "\""); } - std::vector GetGroupsPath( const std::unordered_set& masterlistGroups, const std::unordered_set& userGroups, @@ -264,8 +270,8 @@ std::vector GetGroupsPath( if (logger) { logger->error( "Unreachable vertex {} encountered while looking for vertex {}", - graph[currentVertex].GetName(), - graph[toVertex].GetName()); + graph[currentVertex], + graph[toVertex]); } return std::vector(); } @@ -273,15 +279,15 @@ std::vector GetGroupsPath( auto pair = boost::edge(nextVertex, currentVertex, graph); if (!pair.second) { throw std::runtime_error("Unexpectedly couldn't find edge between \"" + - graph[currentVertex].GetName() + "\" and \"" + - graph[nextVertex].GetName() + "\""); + graph[currentVertex] + "\" and \"" + + graph[nextVertex] + "\""); } - auto vertex = Vertex(graph[currentVertex].GetName(), graph[pair.first]); + auto vertex = Vertex(graph[currentVertex], graph[pair.first]); path.push_back(vertex); currentVertex = nextVertex; } - path.push_back(Vertex(graph[currentVertex].GetName())); + path.push_back(Vertex(graph[currentVertex])); return path; } diff --git a/src/api/sorting/group_sort.h b/src/api/sorting/group_sort.h index 1dde41a2..e05bf206 100644 --- a/src/api/sorting/group_sort.h +++ b/src/api/sorting/group_sort.h @@ -47,49 +47,5 @@ std::vector GetGroupsPath( const std::unordered_set& userGroups, const std::string& fromGroupName, const std::string& toGroupName); - -template -class CycleDetector : public boost::dfs_visitor<> { -public: - void tree_edge(typename boost::graph_traits::edge_descriptor edge, - const G& graph) { - auto source = boost::source(edge, graph); - - auto vertex = Vertex(graph[source].GetName(), graph[edge]); - - // Check if the vertex already exists in the recorded trail. - auto it = find_if(begin(trail), end(trail), [&](const Vertex& v) { - return v.GetName() == graph[source].GetName(); - }); - - if (it != end(trail)) { - // Erase everything from this position onwards, as it doesn't - // contribute to a forward-cycle. - trail.erase(it, end(trail)); - } - - trail.push_back(vertex); - } - - void back_edge(typename boost::graph_traits::edge_descriptor edge, - const G& graph) { - auto source = boost::source(edge, graph); - auto target = boost::target(edge, graph); - - auto vertex = Vertex(graph[source].GetName(), graph[edge]); - trail.push_back(vertex); - - auto it = find_if(begin(trail), end(trail), [&](const Vertex& v) { - return v.GetName() == graph[target].GetName(); - }); - - if (it != trail.end()) { - throw CyclicInteractionError(std::vector(it, trail.end())); - } - } - -private: - std::vector trail; -}; } #endif diff --git a/src/api/sorting/plugin_sorter.cpp b/src/api/sorting/plugin_sorter.cpp index 50e3bf0a..6c5a5caf 100644 --- a/src/api/sorting/plugin_sorter.cpp +++ b/src/api/sorting/plugin_sorter.cpp @@ -49,6 +49,47 @@ typedef boost::graph_traits::vertex_iterator vertex_it; typedef boost::graph_traits::edge_descriptor edge_t; typedef boost::graph_traits::edge_iterator edge_it; +class CycleDetector : public boost::dfs_visitor<> { +public: + void tree_edge(edge_t edge, const PluginGraph& graph) { + auto source = boost::source(edge, graph); + + auto vertex = Vertex(graph[source].GetName(), graph[edge]); + + // Check if the vertex already exists in the recorded trail. + auto it = find_if(begin(trail), end(trail), [&](const Vertex& v) { + return v.GetName() == graph[source].GetName(); + }); + + if (it != end(trail)) { + // Erase everything from this position onwards, as it doesn't + // contribute to a forward-cycle. + trail.erase(it, end(trail)); + } + + trail.push_back(vertex); + } + + void back_edge(edge_t edge, const PluginGraph& graph) { + auto source = boost::source(edge, graph); + auto target = boost::target(edge, graph); + + auto vertex = Vertex(graph[source].GetName(), graph[edge]); + trail.push_back(vertex); + + auto it = find_if(begin(trail), end(trail), [&](const Vertex& v) { + return v.GetName() == graph[target].GetName(); + }); + + if (it != trail.end()) { + throw CyclicInteractionError(std::vector(it, trail.end())); + } + } + +private: + std::vector trail; +}; + std::string describeEdgeType(EdgeType edgeType) { switch (edgeType) { case EdgeType::hardcoded: @@ -264,8 +305,7 @@ void PluginSorter::CheckForCycles() const { logger_->trace("Checking plugin graph for cycles..."); } boost::depth_first_search( - graph_, - visitor(CycleDetector()).vertex_index_map(vertexIndexMap_)); + graph_, visitor(CycleDetector()).vertex_index_map(vertexIndexMap_)); } bool PluginSorter::EdgeCreatesCycle(const vertex_t& fromVertex, diff --git a/src/tests/api/internals/sorting/group_sort_test.h b/src/tests/api/internals/sorting/group_sort_test.h index 9d370f8d..06be32ed 100644 --- a/src/tests/api/internals/sorting/group_sort_test.h +++ b/src/tests/api/internals/sorting/group_sort_test.h @@ -36,9 +36,7 @@ namespace loot { namespace test { TEST(GetTransitiveAfterGroups, shouldMapGroupsToTheirTransitiveAfterGroups) { std::unordered_set groups( - {Group("a"), - Group("b", std::unordered_set({"a"})), - Group("c", std::unordered_set({"b"}))}); + {Group("a"), Group("b", {"a"}), Group("c", {"b"})}); auto mapped = GetTransitiveAfterGroups(groups, {}); @@ -48,18 +46,14 @@ TEST(GetTransitiveAfterGroups, shouldMapGroupsToTheirTransitiveAfterGroups) { } TEST(GetTransitiveAfterGroups, shouldThrowIfAnAfterGroupDoesNotExist) { - std::unordered_set groups( - {Group("b", std::unordered_set({"a"}))}); + std::unordered_set groups({Group("b", {"a"})}); EXPECT_THROW(GetTransitiveAfterGroups(groups, {}), UndefinedGroupError); } TEST(GetTransitiveAfterGroups, shouldThrowIfAfterGroupsAreCyclic) { - std::unordered_set groups( - {Group("a", std::unordered_set({"c"})), - Group("b", std::unordered_set({"a"}))}); - std::unordered_set userGroups( - {Group("c", std::unordered_set({"b"}))}); + std::unordered_set groups({Group("a"), Group("b", {"a"})}); + std::unordered_set userGroups({Group("a", {"c"}), Group("c", {"b"})}); try { GetTransitiveAfterGroups(groups, userGroups); @@ -69,7 +63,7 @@ TEST(GetTransitiveAfterGroups, shouldThrowIfAfterGroupsAreCyclic) { // Vertices can be added in any order, so which group is first is undefined. if (e.GetCycle()[0].GetName() == "a") { - EXPECT_EQ(EdgeType::masterlistLoadAfter, + EXPECT_EQ(EdgeType::userLoadAfter, e.GetCycle()[0].GetTypeOfEdgeToNextVertex()); EXPECT_EQ("c", e.GetCycle()[1].GetName()); @@ -84,7 +78,7 @@ TEST(GetTransitiveAfterGroups, shouldThrowIfAfterGroupsAreCyclic) { e.GetCycle()[0].GetTypeOfEdgeToNextVertex()); EXPECT_EQ("a", e.GetCycle()[1].GetName()); - EXPECT_EQ(EdgeType::masterlistLoadAfter, + EXPECT_EQ(EdgeType::userLoadAfter, e.GetCycle()[1].GetTypeOfEdgeToNextVertex()); EXPECT_EQ("c", e.GetCycle()[2].GetName()); @@ -100,23 +94,23 @@ TEST(GetTransitiveAfterGroups, shouldThrowIfAfterGroupsAreCyclic) { e.GetCycle()[1].GetTypeOfEdgeToNextVertex()); EXPECT_EQ("a", e.GetCycle()[2].GetName()); - EXPECT_EQ(EdgeType::masterlistLoadAfter, + EXPECT_EQ(EdgeType::userLoadAfter, e.GetCycle()[2].GetTypeOfEdgeToNextVertex()); } } } TEST(GetGroupsPath, shouldThrowIfTheFromGroupDoesNotExist) { - std::unordered_set groups({Group("a", {"c"}), Group("b", {"a"})}); - std::unordered_set userGroups({Group("c", {"b"})}); + std::unordered_set groups({Group("a"), Group("b", {"a"})}); + std::unordered_set userGroups({Group("a", {"c"}), Group("c", {"b"})}); EXPECT_THROW(GetGroupsPath(groups, userGroups, "d", "a"), std::invalid_argument); } TEST(GetGroupsPath, shouldThrowIfTheToGroupDoesNotExist) { - std::unordered_set groups({Group("a", {"c"}), Group("b", {"a"})}); - std::unordered_set userGroups({Group("c", {"b"})}); + std::unordered_set groups({Group("a"), Group("b", {"a"})}); + std::unordered_set userGroups({Group("a", {"c"}), Group("c", {"b"})}); EXPECT_THROW(GetGroupsPath(groups, userGroups, "a", "d"), std::invalid_argument); @@ -161,8 +155,8 @@ TEST(GetGroupsPath, std::unordered_set groups({Group("a", {}), Group("b", {"a"}), Group("c", {"a"}), - Group("e", {"b", "d"})}); - std::unordered_set userGroups({Group("d", {"c"})}); + Group("e", {"b"})}); + std::unordered_set userGroups({Group("d", {"c"}), Group("e", {"d"})}); auto path = GetGroupsPath(groups, userGroups, "a", "e"); @@ -174,11 +168,22 @@ TEST(GetGroupsPath, EXPECT_EQ(EdgeType::userLoadAfter, path[1].GetTypeOfEdgeToNextVertex().value()); EXPECT_EQ("d", path[2].GetName()); - EXPECT_EQ(EdgeType::masterlistLoadAfter, + EXPECT_EQ(EdgeType::userLoadAfter, path[2].GetTypeOfEdgeToNextVertex().value()); EXPECT_EQ("e", path[3].GetName()); EXPECT_FALSE(path[3].GetTypeOfEdgeToNextVertex().has_value()); } + +TEST(GetGroupsPath, shouldThrowIfMasterlistGroupLoadsAfterAUserlistGroup) { + std::unordered_set groups({Group("a", {}), + Group("b", {"a"}), + Group("c", {"a"}), + Group("e", {"b", "d"})}); + std::unordered_set userGroups({Group("d", {"c"})}); + + EXPECT_THROW(GetGroupsPath(groups, userGroups, "a", "e"), + UndefinedGroupError); +} } }