[PATCH] D77167: [GraphDiff] Extend GraphDiff to track a list of updates.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/include/llvm/IR/CFGDiff.h:81-84
   UpdateMapType SuccInsert;
   UpdateMapType SuccDelete;
   UpdateMapType PredInsert;
   UpdateMapType PredDelete;
----------------
Perhaps making a struct { Succ, Pred } & then you could even have an array of [2] of these and index into it with isInsert, so delete ones are at index zero and the succeess ones are at index 1. Could make the existing code and the new use of these maps a bit tidier (that if/else around line 156  -separates the initialization from the declarations and duplicates a fair bit of text between the two sides, making it less obvious what's different on either side)


================
Comment at: llvm/include/llvm/Support/CFGUpdate.h:107-119
+  if (!ReverseResultOrder)
+    llvm::
+        sort(Result,
              [&Operations](const Update<NodePtr> &A, const Update<NodePtr> &B) {
                return Operations[{A.getFrom(), A.getTo()}] >
                       Operations[{B.getFrom(), B.getTo()}];
              });
----------------
kuhar wrote:
> nit: this if-else spans 13 lines, consider adding braces
I'd /probably/ rephrase this as:
```
sort(Result, [&] (...) {
  const auto &OpA = Operations...;
  const auto &OpB = ...;
  return ReverseResultOrder ? OpA < OpB
       OpA > OpB;
});
```

Or something roughly like that - as it'd make it easier to see the difference between the two cases rather than trying to look at two blocks of very similar code and spot the </> difference in amongst all the other content.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77167