Page MenuHomePhabricator

Allow BasicBlockEdge to be used in DenseMap
ClosedPublic

Authored by dberlin on Jul 10 2016, 8:48 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

dberlin retitled this revision from to Allow BasicBlockEdge to be used in DenseMap.
dberlin updated this object.

The use case for this is that anything that includes Dominators.h gets this definition of BasicBlockEdge. and it seems like an okay one at a glance, so may as well reuse it. This is what is required to use them as a DenseMap key.

The other easy option i see is to just typedef BasicBlockEdge to std::pair<const BasicBlock *, const BasicBlock *> here, and make isSingleEdge (which has 3 uses in all of llvm) a non-member function.

In fact, all the code for our DenseMapInfo is just being reused from the existing std::pair specialization DenseMapInfo has.

Not sure which we prefer, happy to do either

hfinkel edited edge metadata.Jul 10 2016, 8:55 PM

The use case for this is that anything that includes Dominators.h gets this definition of BasicBlockEdge. and it seems like an okay one at a glance, so may as well reuse it. This is what is required to use them as a DenseMap key.

The other easy option i see is to just typedef BasicBlockEdge to std::pair<const BasicBlock *, const BasicBlock *> here, and make isSingleEdge (which has 3 uses in all of llvm) a non-member function.

In fact, all the code for our DenseMapInfo is just being reused from the existing std::pair specialization DenseMapInfo has.

Not sure which we prefer, happy to do either

What would happen if you made BasicBlockEdge convertible into a std::pair<const BasicBlock *, const BasicBlock *>? Would you still need this?

hfinkel accepted this revision.Jul 10 2016, 9:12 PM
hfinkel edited edge metadata.

The use case for this is that anything that includes Dominators.h gets this definition of BasicBlockEdge. and it seems like an okay one at a glance, so may as well reuse it. This is what is required to use them as a DenseMap key.

The other easy option i see is to just typedef BasicBlockEdge to std::pair<const BasicBlock *, const BasicBlock *> here, and make isSingleEdge (which has 3 uses in all of llvm) a non-member function.

In fact, all the code for our DenseMapInfo is just being reused from the existing std::pair specialization DenseMapInfo has.

Not sure which we prefer, happy to do either

What would happen if you made BasicBlockEdge convertible into a std::pair<const BasicBlock *, const BasicBlock *>? Would you still need this?

Given that the conversion does not work, this LGTM. I'd rather have real names for the endpoints (not just first and second as we'd get if we use pair).

This revision is now accepted and ready to land.Jul 10 2016, 9:12 PM
This revision was automatically updated to reflect the committed changes.