[PATCH] D123677: [PassManager] Implement DOTGraphTraitsViewer under NPM

[Min-Yih Hsu via Phabricator via llvm-commits]

myhsu added a comment.

I understand that most of the code was copied from the original (LegacyPM) implementation, but I feel like it's also a good time to fix some of the minor issues as highlighted by my inline comments.



================
Comment at: llvm/include/llvm/Analysis/DOTGraphTraitsPass.h:77
+                                                 AnalysisGraphTraitsT>> {
+public:
+  DOTGraphTraitsPrinter(StringRef GraphName) : Name(GraphName) {}
----------------
could we use struct and remove this?


================
Comment at: llvm/include/llvm/Analysis/DOTGraphTraitsPass.h:91
+
+  PreservedAnalyses run(llvm::Function &F, llvm::FunctionAnalysisManager &FAM) {
+    auto &Result = FAM.getResult<AnalysisT>(F);
----------------
the "llvm::" is redundant since we're already enclosed in namespace llvm


================
Comment at: llvm/include/llvm/Analysis/DOTGraphTraitsPass.h:104
+    std::string GraphName = DOTGraphTraits<GraphT>::getGraphName(Graph);
+    std::string Title = GraphName + " for '" + F.getName().str() + "' function";
+
----------------
Please use Twine here. Also, F.getName() is sufficient, no need to create another std::string (via str())


================
Comment at: llvm/include/llvm/Analysis/DOTGraphTraitsPass.h:116
+private:
+  std::string Name;
+};
----------------
Is there any specific reason we want to own the string? Is it possible to replace it with StringRef?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123677