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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
