From 7d344fb08ed1a5bdb109feeb9f703a2581378fb4 Mon Sep 17 00:00:00 2001 From: Toni Dietze Date: Tue, 23 Feb 2021 23:04:18 +0100 Subject: [PATCH] logic: fix multi-level inheritance edges when there are diamonds (#1142) The old implementation produces several multi-inheritance edges between two classes if there are multiple inheritance paths between them due to multiple inheritance diamonds. The new implementation produces only one edge in such a case. Also, the old implementation has performance issues when there is a huge number of inheritance paths starting at the focused class. With a pathological multiple inheritance class structure, the number of paths can be exponential in the number of involved classes. The new implementation solves this issue by considering subgraphs instead of paths. The following is a small problematic example, which does not show the performance problem due to its small size. However, when focusing on X::foo, the old implementation produces 4 different multi-level inheritance edges from X to B1 while the new implementation produces only one multi-level inheritance edge, which is basically the union of the edges of the old implementation: struct B1 { virtual void foo() = 0; }; struct C1 : B1 {}; struct D1 : B1 {}; struct B2 : C1, D1 {}; struct C2 : B2 {}; struct D2 : B2 {}; struct B3 : C2, D2 {}; struct X : B3 { void foo() override {}; }; int main () { X x; x.foo(); return 0; } --- src/lib/data/HierarchyCache.cpp | 115 +++++++++++++++++++++++++------- src/lib/data/HierarchyCache.h | 43 ++++++++++-- 2 files changed, 126 insertions(+), 32 deletions(-) diff --git a/src/lib/data/HierarchyCache.cpp b/src/lib/data/HierarchyCache.cpp index 6eedf7fc..65c6e27e 100644 --- a/src/lib/data/HierarchyCache.cpp +++ b/src/lib/data/HierarchyCache.cpp @@ -114,31 +114,27 @@ void HierarchyCache::HierarchyNode::setIsImplicit(bool isImplicit) m_isImplicit = isImplicit; } -void HierarchyCache::HierarchyNode::addInheritanceEdgesRecursive( - Id startId, - const std::set& inheritanceEdgeIds, - const std::set& nodeIds, - std::vector>>* inheritanceEdges) +std::map>> +HierarchyCache::HierarchyNode::getReverseReachableInheritanceSubgraph() const { - for (size_t i = 0; i < m_bases.size(); i++) + std::map>> reverseGraph; + reverseGraph.try_emplace(getNodeId()); // mark start node as visited + getReverseReachableInheritanceSubgraphHelper(reverseGraph); + return reverseGraph; +} + +void HierarchyCache::HierarchyNode::getReverseReachableInheritanceSubgraphHelper( + std::map>>& reverseGraph) const +{ + for (size_t i = 0; i < m_bases.size(); ++i) { - if (inheritanceEdgeIds.find(m_baseEdgeIds[i]) != inheritanceEdgeIds.end()) - { - continue; - } - HierarchyNode* base = m_bases[i]; - Id baseId = base->getNodeId(); - - std::set inheritanceEdgeIds2 = inheritanceEdgeIds; - inheritanceEdgeIds2.insert(m_baseEdgeIds[i]); - - if (nodeIds.find(baseId) != nodeIds.end()) + auto emplacedBase = reverseGraph.try_emplace(base->getNodeId()); + emplacedBase.first->second.push_back({getNodeId(), m_baseEdgeIds[i]}); + if (emplacedBase.second) { - inheritanceEdges->push_back({startId, baseId, utility::toVector(inheritanceEdgeIds2)}); + base->getReverseReachableInheritanceSubgraphHelper(reverseGraph); } - - base->addInheritanceEdgesRecursive(startId, inheritanceEdgeIds2, nodeIds, inheritanceEdges); } } @@ -340,20 +336,89 @@ bool HierarchyCache::nodeIsImplicit(Id nodeId) const return false; } -std::vector>> HierarchyCache::getInheritanceEdgesForNodeId( - Id nodeId, const std::set& nodeIds) const +std::vector>> +HierarchyCache::getInheritanceEdgesForNodeId( + Id sourceId, const std::set& targetIds) const { + // For two nodes s and t of a graph g0, this function determines the subgraph g2 that consists + // of all nodes and edges that are reachable by going from s forwards and from t backwards as + // follows: First the subgraph g1 that consists of all nodes and edges that are reachable from s + // is determined. Afterwards g2 is determined by keeping only those nodes and edges of g1 that + // are reachable by going from t backwards. If t is not in g1, then the g2 is empty. + // + // For example (edges are pointing upwards): + // + // g0 * g1 * g2 + // | | + // t t t + // | | | + // * * * * * + // \ / \ \ / \ / \ + // * * * * * * + // \ / \ \ / \ / + // * * * * + // | | | + // s s s + // | + // * + std::vector>> inheritanceEdges; - HierarchyNode* node = getNode(nodeId); - if (node) + if (targetIds.empty()) { - node->addInheritanceEdgesRecursive(node->getNodeId(), {}, nodeIds, &inheritanceEdges); + return inheritanceEdges; + } + + HierarchyNode* sourceNode = getNode(sourceId); + if (!sourceNode) + { + return inheritanceEdges; + } + + std::map>> reverseGraph + = sourceNode->getReverseReachableInheritanceSubgraph(); + + for (Id targetId : targetIds) + { + std::set nodes; + std::vector edges; + getReverseReachable(targetId, reverseGraph, nodes, edges); + + if (!edges.empty()) + { + inheritanceEdges.push_back({sourceId, targetId, std::move(edges)}); + } } return inheritanceEdges; } +void HierarchyCache::getReverseReachable( + Id nodeId, + const std::map>>& reverseGraph, + std::set& nodes, + std::vector& edges) +{ + if (!nodes.insert(nodeId).second) + { + return; + } + + auto search = reverseGraph.find(nodeId); + if (search == reverseGraph.end()) + { + return; + } + + for (const std::pair& nodeAndEdge : search->second) + { + Id node = nodeAndEdge.first; + Id edge = nodeAndEdge.second; + edges.push_back(edge); + getReverseReachable(node, reverseGraph, nodes, edges); + } +} + HierarchyCache::HierarchyNode* HierarchyCache::getNode(Id nodeId) const { auto it = m_nodes.find(nodeId); diff --git a/src/lib/data/HierarchyCache.h b/src/lib/data/HierarchyCache.h index 6d1230e3..873938db 100644 --- a/src/lib/data/HierarchyCache.h +++ b/src/lib/data/HierarchyCache.h @@ -33,10 +33,29 @@ public: bool nodeIsVisible(Id nodeId) const; bool nodeIsImplicit(Id nodeId) const; - std::vector>> getInheritanceEdgesForNodeId( - Id nodeId, const std::set& nodeIds) const; + std::vector>> + getInheritanceEdgesForNodeId(Id sourceId, const std::set& targetIds) const; private: + /** + * Determine nodes and edges from which a specific node can be reached in a reversed graph. + * + * A reversed graph can be produced by HierarchyNode::getReverseReachableInheritanceSubgraph(). + * + * @param[in] nodeId ID of the target node. + * @param[in] reverseGraph The reversed graph. + * @param[out] nodes The nodes from which the node @p nodeId can be reached. + * @param[out] edges The edges from which the node @p nodeId can be reached. + * + * @pre The arguments for @p nodes and @p edges must be provided empty. + */ + static void getReverseReachable( + Id nodeId, + const std::map>>& + reverseGraph, + std::set& nodes, + std::vector& edges); + class HierarchyNode { public: @@ -67,13 +86,23 @@ private: bool isImplicit() const; void setIsImplicit(bool isImplicit); - void addInheritanceEdgesRecursive( - Id startId, - const std::set& inheritanceEdgeIds, - const std::set& nodeIds, - std::vector>>* inheritanceEdges); + /** + * Determine the reversed subgraph of all nodes and edges that are reachable from this node. + * + * The subgraph is represented by a map that maps a node ID *t* to a set of pairs where each + * pair consists of a node ID *s* and and edge ID *e* such that *e* refers to an edge from + * *s* to *t*. Note that the mapping is reversed compared to the edges. + */ + std::map>> + getReverseReachableInheritanceSubgraph() const; private: + /** + * Helper for getReverseReachableInheritanceSubgraph(). + */ + void getReverseReachableInheritanceSubgraphHelper( + std::map>>&) const; + const Id m_nodeId; Id m_edgeId;