[CGQuery] Refactor & add (Post) Dominator analysis#111
[CGQuery] Refactor & add (Post) Dominator analysis#111silas-martens wants to merge 9 commits intotudasc:develfrom
Conversation
|
Is this still a draft, or are you looking for feedback? |
Im looking for feedback :) |
|
The analysis should have unit tests. |
sebastiankreutzer
left a comment
sebastiankreutzer
left a comment
There was a problem hiding this comment.
Looks good, but I agree with JP that there should be rudimentary unit tests.
|
|
||
| auto& doms_node1 = doms[node1].Doms; | ||
| ASSERT_TRUE(doms_node1.size() == 2); | ||
| ASSERT_TRUE(doms_node1.count(main)); |
There was a problem hiding this comment.
Why ASSERT_TRUE on something that supposedly returns a number? Is all we care that the number is != 0?
| #include <string> | ||
| #include <vector> | ||
|
|
||
| struct Command { |
| return !s.empty() && std::all_of(s.begin(), s.end(), [](unsigned char c) { return std::isdigit(c); }); | ||
| } | ||
|
|
||
| inline metacg::CgNode* findNode(metacg::Callgraph* cg, const std::string& spec, const std::string& role) { |
There was a problem hiding this comment.
I think we already have a graph utils header in the main lib. What is the reason this is here?
| #include "commands/ReachesCommand.h" | ||
| #include "utils.h" | ||
|
|
||
| #include <cxxopts.hpp> |
| #include <cxxopts.hpp> | ||
| #include <iostream> | ||
| #include <memory> | ||
| #include <ostream> |
There was a problem hiding this comment.
Do we need this when iostream is already there?
| DomAnalysisResult<NodeT> computeDoms(const GraphT& graph, const NodeT& exitNode); | ||
|
|
||
| template <typename Container> | ||
| Container unordered_intersection(const Container& a, const Container& b) { |
| using DomAnalysisResult = std::unordered_map<const NodeT*, DomData<NodeT>>; | ||
|
|
||
| template <typename NodeT, typename GraphT> | ||
| DomAnalysisResult<NodeT> computeDoms(const GraphT& graph, const NodeT& exitNode); |
There was a problem hiding this comment.
What are people opinions on renaming computeDominators? I would think we can afford the additional 6 characters.
| } | ||
|
|
||
| template <typename NodeT, typename GraphT, TraverseDir dir> | ||
| DomAnalysisResult<NodeT> computeDoms(const GraphT& graph, const NodeT& exitNode) { |
| return graph.getCallees(*n); | ||
| }; | ||
|
|
||
| using DomDataT = DomData<NodeT>; |
There was a problem hiding this comment.
Should this move to beginning of scope?
| typename DomDataT::NodeSet DomNew = intersection(initializedCallees, DomMap); | ||
| DomNew.insert(nodeData.node); | ||
|
|
||
| // LOG_STATUS("New Doms " << dumpNodeSet(nodeData.node->getName(), DomNew) << "\n"); |
4 participants
This PR introduces a refactoring of CGQuery and adds Dominator and Post-dominator analysis.
The refactoring separates each analysis into its own command which cleans up the main file and is more extensible.