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

[Alina Sbirlea via Phabricator via cfe-commits]

asbirlea marked 2 inline comments as done.
asbirlea added a comment.

Thank you for all the comments.



================
Comment at: llvm/include/llvm/Support/GenericDomTreeConstruction.h:325
+    auto &GD = BUI ? BUI->PreViewCFG : EmptyGD;
+    return !empty(children<GraphDiffNodePair>({&GD, N}));
   }
----------------
kuhar wrote:
> 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.
> Does allocating a GD have a big cost?
AFAIU, dynamically allocating the object does, vs having a stack variable.


I'll do a follow-up cleanup attempt.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77341