[PATCH] D91327: [NewPM] Redesign of PreserveCFG Checker

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar added a comment.

Found some cosmetics, but I'm not familiar enough with the NPM to do a full review.



================
Comment at: llvm/include/llvm/Passes/StandardInstrumentations.h:99
   struct CFG {
     struct BBGuard final : public CallbackVH {
       BBGuard(const BasicBlock *BB) : CallbackVH(BB) {}
----------------
Consider pulling this out of CFG. I don't see many reasons for having 3 levels of class nesting.


================
Comment at: llvm/include/llvm/Passes/StandardInstrumentations.h:134
 
 public:
   static cl::opt<bool> VerifyPreservedCFG;
----------------
not necessary anymore


================
Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:728
+  Result run(Function &F, FunctionAnalysisManager &AM) {
+    return Result(&F, true /* TrackBBLifetime */);
+  }
----------------
nit: move parameter name to the left?


================
Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:759
+    CFG::printDiff(dbgs(), GraphBefore, GraphAfter);
+    report_fatal_error(Twine("CFG unexpectedly changed by ", Pass));
+  };
----------------
I think this will print with `errs()`. Would it make sense to flush `dbgs()` ahead of printing with `errs()`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91327