Details
Diff Detail
Event Timeline
include/llvm/Analysis/BlockFrequencyInfoImpl.h | ||
---|---|---|
1274 | I'm confused by this '&'. If GraphTraits is meant to be node handle agnostic, why is it producing a dereferenced node handle type here? (such that the address needs to be taken again) | |
include/llvm/Analysis/RegionIterator.h | ||
53 | This would still be restricted to pointers - so it doesn't actually generalize over the new node handles you are introducing. Does this need deeper changes? | |
277–305 | Might be worth reformatting this in a separate preliminary commit (no review required, jsut mash clang-format on that, commit, then resolve the changes against this commit/code review) |
Added static_assert.
include/llvm/Analysis/BlockFrequencyInfoImpl.h | ||
---|---|---|
1274 | This is more or less embarrassing: Some implementors decide to let *I return NodeType& (e.g. I as Function::iterator), while others decide to let *I return NodeType*. GraphWriter has the same problem. The solution of GraphWriter (even before the NodeType -> NodeRef change) is to pass *I into Foo, where Foo is overloaded on NodeType&, NodeType* and NodeType **. | |
include/llvm/Analysis/RegionIterator.h | ||
53 | As discussed offline, put a static_assert here. |
include/llvm/Analysis/RegionIterator.h | ||
---|---|---|
35 | If it has to be a pointer type anyway - would it be simpler to preserve that behavior of value_type being a pointer rather than a value? Or is there something else going on here as well that makes this change necessary/useful? |
include/llvm/Analysis/RegionIterator.h | ||
---|---|---|
35 | Actually by looking at the code, I can tell that NodeType is always GraphTraits<BlockT*>::NodeType. After the migration I prefer to keep the consistency, that is the template parameter NodeRef is always GraphTraits<BlockT*>::NodeRef. The fact that NodeRef is required to be a pointer seems not worthy to be mentioned too frequently. The code also assumes that BlockT* is NodeRef/NodeType*. And the code also uses getParent(). I'm sad that people stick on GraphTraits just because it exists, when TreeTraits could be a better fit. |
Hi Tim,
thank you for looking through this. The general direction and the changes look fine for RegionInfo. I let David finish his detailed comments (thank you David!) for him to give the final OK.
include/llvm/Analysis/RegionIterator.h | ||
---|---|---|
53 | This seems a good solution for now. This PointerIntPair should just be replaced with two separate fields, but can be done later on. However, this will not be sufficient to allow pointer types, as there seem to be other uses of PtrSet, which would need to be changed. I assume we leave this for when there is an actual need for a non-pointer type. |
I'm confused by this '&'.
If GraphTraits is meant to be node handle agnostic, why is it producing a dereferenced node handle type here? (such that the address needs to be taken again)