[PATCH] D29005: [XRay] A graph Class for the llvm-xray graph
Dean Michael Berris via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Jan 23 22:51:05 PST 2017
dberris added a comment.
As @dblaikie already mentioned, it's a really good idea to give this a good set of unit tests given the complexity of the implementation of this type.
================
Comment at: include/llvm/XRay/Graph.h:26-29
+namespace llvm{
+namespace xray{
+
+namespace internal{
----------------
Please put one space between the namespace name and the opening `{`.
================
Comment at: include/llvm/XRay/Graph.h:32
+/// An iterator Class which iterates using BaseIt, using a conversion function
+/// to dereference the poiter.
+template <class BaseIt,
----------------
s/poiter/pointer/
================
Comment at: include/llvm/XRay/Graph.h:37
+ typename ReturnCollection = LookupCollection>
+class ProxiedIterator {
+ typedef ProxiedIterator<BaseIt, const LookupCollection, T, const ReturnCollection> ConstIterator;
----------------
The name seems to be misleading. I think this is a transforming iterator (applies a function on dereference). A lot of the machinery being implemented here seems unnecessary if you define this in terms of the already existing iterator adapter type in llvm/ADT/iterator.h -- have you considered doing that either for this iterator?
================
Comment at: include/llvm/XRay/Graph.h:66
+ public:
+
+ template<class Other,
----------------
Unnecessary empty line?
================
Comment at: include/llvm/XRay/Graph.h:77-79
+ H = [&I](BaseIt It, LookupCollection &C, T Id) -> RetIt{
+ return I.H(It, const_cast<typename std::remove_const<LookupCollection>::type &>(C), Id);
+ };
----------------
I don't think you need this defined in the body of the constructor -- you can initialise H directly with this lambda.
================
Comment at: include/llvm/XRay/Graph.h:80
+ };
+ };
+
----------------
Spurious semicolon.
================
Comment at: include/llvm/XRay/Graph.h:82
+
+ ProxiedIterator(): C(nullptr){H = nullptr;};
+
----------------
You can probably just use non-static data member initialisation for these (instead of having a mix of an initialiser list and an in-constructor definition). That way you can `=default` this constructor instead.
Also you have a spurious semicolon at the end of a function definition.
================
Comment at: include/llvm/XRay/Graph.h:84-85
+
+ ProxiedIterator(BaseIt _B, LookupCollection &_C,
+ ConversionHelper _H, T _S): B(_B), C(&_C), S(_S), H(_H){};
+
----------------
Please avoid using names with leading underscores.
================
Comment at: include/llvm/XRay/Graph.h:85
+ ProxiedIterator(BaseIt _B, LookupCollection &_C,
+ ConversionHelper _H, T _S): B(_B), C(&_C), S(_S), H(_H){};
+
----------------
Spurious semicolon.
================
Comment at: include/llvm/XRay/Graph.h:97
+ return B != b.B;
+ }
+ bool operator==(const ConstIterator &b) const{
----------------
Empty line after this?
================
Comment at: include/llvm/XRay/Graph.h:105
+ return *this;
+ }
+ ProxiedIterator operator++(int){
----------------
Empty line after this?
================
Comment at: include/llvm/XRay/Graph.h:115-120
+template <typename VertexAttribute,
+ typename EdgeAttribute,
+ typename GraphAttribute,
+ typename VI = int32_t,
+ typename EI = std::pair<VI,VI>>
+class Graph : public GraphAttribute{
----------------
For types that are user-facing, it's usually preferred that it's documented to at least contain the following:
- Important semantic concepts. In this domain (of graphs), say what kind of graph this represents (labelled directed graph).
- Usage examples (how to use this, say as how library users might use it).
- Requirements on the parametric types, as this is a template.
================
Comment at: include/llvm/XRay/Graph.h:120
+ typename EI = std::pair<VI,VI>>
+class Graph : public GraphAttribute{
+ public:
----------------
Space between identifier and {?
================
Comment at: include/llvm/XRay/Graph.h:130
+ /// easy iteration and iterators.
+ struct EdgeValueType : public std::pair<EdgeIdentifier, EdgeAttribute>{
+ VertexIdentifier &getFirst() {
----------------
Why the inheritance? Will this not work with composition instead? Or is this for the empty base optimisation?
================
Comment at: include/llvm/XRay/Graph.h:131-142
+ VertexIdentifier &getFirst() {
+ return std::pair<EdgeIdentifier, EdgeAttribute>::first.second;
+ };
+ const VertexIdentifier &getFirst() const{
+ return std::pair<EdgeIdentifier, EdgeAttribute>::first.second;
+ };
+ EdgeAttribute &getSecond(){
----------------
Since you're giving names anyway, consider using more descriptive ones (like `getVertexId` and `getEdgeAttr`).
================
Comment at: include/llvm/XRay/Graph.h:145-163
+ ///Typedefs of the various structure types in this class.
+ typedef class DenseMap<VertexIdentifier,
+ EdgeAttribute,
+ DenseMapInfo<VertexIdentifier>,
+ EdgeValueType> InnerEdgeMapT;
+ typedef class DenseMap<VertexIdentifier, InnerEdgeMapT> EdgeMapT;
+ typedef class DenseMap<VertexIdentifier, VertexAttribute> VertexMapT;
----------------
I may be wrong, but there seems to be a slight preference for `using` instead of `typedef` now.
================
Comment at: include/llvm/XRay/Graph.h:166-170
+ /// The actual data in the graph.
+ EdgeMapT Edges;
+ VertexMapT Vertices;
+ InvGraphT InvGraph;
+ size_type NumEdges;
----------------
Probably document these a little better?
================
Comment at: include/llvm/XRay/Graph.h:184-222
+ template <bool isConst>
+ class InEdgeIteratorProxy {
+ public:
+ typedef typename std::conditional<isConst, ConstInEdgeIterator, InEdgeIterator>::type Iterator;
+ typedef typename std::conditional<isConst, const Graph, Graph>::type GraphT;
+ typedef typename InnerInvGraphT::const_iterator BaseIt;
+ typedef typename std::conditional<isConst, const EdgeMapT, EdgeMapT>::type IntEdgeMapT;
----------------
This doesn't seem like an iterator to me, so the name is a little misleading. This seems to be more of a `View` into the data, or a range.
================
Comment at: include/llvm/XRay/Graph.h:224-245
+ template <bool isConst>
+ class VertexProxy {
+ public:
+ typedef typename std::conditional<isConst, ConstVertexIterator, VertexIterator>::type Iterator;
+ typedef typename std::conditional<isConst, const Graph, Graph>::type GraphT;
+ private:
+ GraphT &G;
----------------
I think you want to call this a `view` as well, instead of a proxy.
================
Comment at: include/llvm/XRay/Graph.h:241
+ }
+ bool empty(){
+ return G.Vertices.empty();
----------------
This function may be always const?
================
Comment at: include/llvm/XRay/Graph.h:287-289
+ // Conveting ctor from non-const iterators to const iterators. SFINAE'd out
+ // for const iterator destinations so it doesn't end up as a user defined
+ // copy constructor.
----------------
Typos?
================
Comment at: include/llvm/XRay/Graph.h:312
+
+ EdgeListIterator& operator ++(){ //preincrement
+ ++BotIt;
----------------
Should be no space between operator and `++`?
================
Comment at: include/llvm/XRay/Graph.h:336-357
+ class EdgeProxy {
+ public:
+ typedef typename std::conditional<isConst, ConstEdgeIterator, EdgeIterator>::type Iterator;
+ typedef typename std::conditional<isConst, const Graph, Graph>::type GraphT;
+ private:
+ GraphT &G;
+ public:
----------------
I think this really ought to be named a `View`.
================
Comment at: include/llvm/XRay/Graph.h:432-435
+ for(auto &V : b.vertices())
+ insert(V);
+ for(auto &E : b.edges())
+ insert(E);
----------------
Is this necessary to implement this way? Can we get away with making copies of the underlying data instead?
I mean, wouldn't it be valid if we just kept this `= default` also?
================
Comment at: include/llvm/XRay/Graph.h:480
+ p.first->first.first = Val.first.first;
+ ++NumEdges;
+ return {EdgeIterator(TI, Edges.end(), p.first), p.second};
----------------
I'm wondering whether you need to keep track of this value yourself (or whether it's something one of the data structures already keeps track of, that you can just return)?
================
Comment at: tools/llvm-xray/xray-graph.h:79
- TimeStat GraphEdgeMax;
- TimeStat GraphVertexMax;
+ typedef Graph<VertexAttribute, EdgeAttribute, GraphAttribute, int32_t> GraphT;
+ GraphT G;
----------------
Consider using better names than generic terms like `VertexAttribute` -- in the context of XRay this might be `FunctionStats`, and `EdgeAttribute` might better be `CallStats`.
https://reviews.llvm.org/D29005
More information about the llvm-commits
mailing list