[PATCH] D77341: [DomTree] Replace ChildrenGetter with GraphTraits over GraphDiff.

[Jakub Kuderski via Phabricator via llvm-commits]

kuhar accepted this revision.
kuhar added a comment.
This revision is now accepted and ready to land.

Looks good to me overall. I don't want to block it over the cosmetic issues like allocating the empty GD object.



================
Comment at: llvm/include/llvm/Support/GenericDomTreeConstruction.h:325
+    auto &GD = BUI ? BUI->PreViewCFG : EmptyGD;
+    return !empty(children<GraphDiffNodePair>({&GD, N}));
   }
----------------
asbirlea wrote:
> kuhar wrote:
> > This pattern repeats a few times. Could it be pushed into a helper function to get something like this?
> > 
> > ```
> > return !empty(children<GraphDiffNodePair>(GetGraphDiffNodePair(BUI, N)));
> > ```
> > 
> My dilemma here is how to not allocate a GraphDiffT instance. There are 4 cases where the pattern is inside a static method and once in a class method. I used a stack var for the 4 cases and a unique_ptr for the class method.
> 
> Sure, I can do:
> ```
> static auto getGDNPair(BUI, EmptyGD, N) {
> return std::make_pair<>(BUI ? BUI->PreViewCFG : EmptyGD, N);
> }
> {
> ...
> GraphDiffT EmptyGD;
> return !empty(children<GraphDiffNodePair>(getGDNPair(BUI, &EmptyGD, N)));
> }
> ```
> But it felt like moving one line at the callsite to a oneline helper is not making things much better.
> 
> 
> I was hoping for something more along the lines of:
> ```
> return !empty(children<GraphDiffNodePair>({getGD(BUI), N});
> ```
> Or, ensure BUI is always non-null, and simplify to:
> ```
> return !empty(children<GraphDiffNodePair>({BUI->PreViewCFG, N});
> ```
> 
> 
> Thoughts?
Does allocating a GD have a big cost?

I think you could get away with returning a temporary GD object that would die as soon as the expression evaluation ends -- should be no different than placing it on the stack just before the function call.

Overall, I still don't understand why you try to avoid creating a wrapper function / struct that returns children, and call `childredn<...>(...)` directly. Either way, I being able to write:
```
return !empty(children<GraphDiffNodePair>({getGD(BUI), N});
```
or 
```
return !empty(getChildren<GraphDiffNodePair>(BUI, N));
```
would definitely be concise and readable enough IMO.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77341/new/

https://reviews.llvm.org/D77341