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

David Blaikie via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Apr 2 16:50:20 PDT 2020


dblaikie added a comment.

Couple of small nits, but I'll leave most of the review to someoen else here - I /think/ it's beyond my context/experience (but if necessary, poke me and I can look more/ask more questions/etc)



================
Comment at: llvm/include/llvm/Support/GenericDomTreeConstruction.h:324-325
+    const GraphDiffT *GDTmp = BUI ? BUI->PreViewOfCFG : &EmptyGD;
+    std::pair<const GraphDiffT *, NodePtr> GDNodePair =
+        std::make_pair(GDTmp, N);
+    for (auto &Pair : children<GraphDiffBBPair>(GDNodePair)) {
----------------
No need for make_pair if you're going to specify the types on the LHS, could write this as:
```
std::pair<...> GDNodePair(GDTmp, N);
```
(similarly two instances further down in this patch - make_pair or explicit type, not both)


Or you could use auto on the LHS given the types of the two parameters seem close enough that it's not too surprising what make_pair produces, maybe even just roll it all in together:
```
return !empty(children<GraphDiffBBPair>(std::make_pair(GDTmp, N)));
```


================
Comment at: llvm/include/llvm/Support/GenericDomTreeConstruction.h:326-330
+    for (auto &Pair : children<GraphDiffBBPair>(GDNodePair)) {
+      (void)Pair;
+      return true;
+    }
+    return false;
----------------
Probably write this as (if I'm understanding the code/intent correctly):
```
return !empty(children<GraphDiffBBPair>(GDNodePair));
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77341





More information about the llvm-commits mailing list