nhaehnle added a comment. In D83088#2213886 <https://reviews.llvm.org/D83088#2213886>, @dblaikie wrote:
> In D83088#2213864 <https://reviews.llvm.org/D83088#2213864>, @nhaehnle wrote: > >> In D83088#2213802 <https://reviews.llvm.org/D83088#2213802>, @dblaikie wrote: >> >>> In D83088#2213797 <https://reviews.llvm.org/D83088#2213797>, @nhaehnle >>> wrote: >>> >>>> In D83088#2208611 <https://reviews.llvm.org/D83088#2208611>, @dblaikie >>>> wrote: >>>> >>>>> This seems like a strange hybrid between a static-polymorphism (with >>>>> traits) and dynamic polymorphism (with base classes/virtual functions). >>>>> Could this more readily be just one or the other? (sounds like you're >>>>> leaning towards dynamic polymorphism) >>>> >>>> No, it's very much this way on purpose. The idea is to support the same >>>> set of functionality as much as possible in both static **and** dynamic >>>> polymorphism. >>> >>> Could it be implemented statically as a primary interface, with a dynamic >>> wrapper? (eg: a base class, then a derived class template that takes the >>> static CFG type to wrap into the dynamic type) keeping the two concepts >>> more clearly separated? >> >> That is how it is implemented. CfgTraits is the primary static interface, >> and then CfgInterface / CfgInterfaceImpl is the dynamic wrapper. > > Ah, fair enough. The inheritance details in the traits class confused me a > bit/I had a hard time following, with all the features being in the one > patch. Might be easier separately, but not sure. > > Would it be possible for this not to use traits - I know @asbirlea and I had > trouble with some things using GraphTraits owing to the traits API. An > alternative would be to describe a CFGGraph concept (same as a standard > container concept, for instance) - where there is a concrete graph object and > that object is queried for things like nodes, edges, etc. (actually one of > the significant things we tripped over was the API choice to navigate edges > from a node itself without any extra state - which meant nodes/edge iteration > had to carry state (potentially pointers back to the graph, etc) to be able > to manifest their edges - trait or concept could both address this by, for > traits, passing the graph as well as the node when querying the trait for > edges, or for a concept passing the node back to the graph to query for > edges). So there is a bit of a part here where I may admittedly be a bit confused with the C++ lingo, since I don't actually like template programming that much :) (Which is part of the motivation for this to begin with... so that I can do the later changes in the stack here without *everything* being in templates.) The way the `CfgTraits` is used is that you never use the `CfgTraits` class directly except to inherit from it using CRTP (curiously recurring template pattern). When writing algorithms that want to be generic over the type of CFG, those algorithms then have a derived class of CfgTraits as a template parameter. For example, D83094 <https://reviews.llvm.org/D83094> adds a `GenericCycleInfo<CfgTraitsT>` template class, where the template parameter should be set to e.g. `IrCfgTraits`, if you want cycle info on LLVM IR, or to `MachineCfgTraits`, if you want cycle info on MachineIR. Both of these classes are derived from `CfgTraits`. It is definitely different from how `GraphTraits` works, which you use it as `GraphTraits<NodeType>`, and then `GraphTraits<BasicBlock *>` etc. are specialized implementations. If `GraphTraits` worked the way that `CfgTraits` works, then we'd instead have classes like `BasicBlockGraphTraits`. So to sum it up, all this sounds a bit to me like maybe calling `CfgTraits` "traits" is wrong? Is that what you're saying here? You can't just call it `Cfg` though, because it's *not* a CFG -- it's a kind of interface to a CFG which is designed for static polymorphism, unlike `CfgInterface` which is designed for dynamic polymorphism. Getting the names right is important, unfortunately I admit that I'm a bit lost there. "Traits" seemed like the closest thing to what I want, but I'm definitely open to suggestions. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83088/new/ https://reviews.llvm.org/D83088 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
