[PATCH] D87202: Add a new hidden option -cfg-dot-changed which creates a website of DOT files showing colourized changes as the IR is changed by passes in the new pass manager pipeline.

[Madhur Amilkanthwar via Phabricator via llvm-commits]

madhur13490 added inline comments.


================
Comment at: llvm/lib/IR/ChangeReporters.cpp:408
+  static int FD[2]{-1, -1};
+  for (int I = 0; I < 2; ++I) {
+    if (FD[I] == -1) {
----------------
I can be unsigned


================
Comment at: llvm/lib/IR/ChangeReporters.cpp:705
+  // in which to store the results.
+  CFGDotDiff(StringRef Title, const BlockSuccessorsFuncData &Before,
+             const BlockSuccessorsFuncData &After, StringRef Dir);
----------------
Doxygen comments can be on newlines.


================
Comment at: llvm/lib/IR/ChangeReporters.cpp:707
+             const BlockSuccessorsFuncData &After, StringRef Dir);
+  // Only needed because of opt bug...
+  ~CFGDotDiff() {
----------------
Is it worth filing a bug report for opt bug and refer it here?


================
Comment at: llvm/lib/IR/ChangeReporters.cpp:779
+    StringRef SR[2];
+    for (int I = 0; I < 2; ++I) {
+      SR[I] = Data[I]->getBody();
----------------
I can be unsigned.


================
Comment at: llvm/lib/IR/ChangeReporters.cpp:795
+    std::string Diff = Data[0]->getLabel().str();
+    Diff = Diff + ":\n<BR align=\"left\"/>" +
+           doLinuxDiff(CFGDotComparer::makeHTMLReady(SR[0]),
----------------
Can use shorthand operator.


================
Comment at: llvm/lib/IR/ChangeReporters.cpp:805
+      size_t P = Diff.find(S);
+      while (P != std::string::npos) {
+        Diff.erase(P, S.length());
----------------
I think you can use C++ regex here. You can get rid of whole while loop and do the intended thing in one line.


================
Comment at: llvm/lib/IR/ChangeReporters.cpp:830
+  S.append(Label.str());
+  S.append(":");
+
----------------
Is there any reason to use series of appends than just a single concatenation?


================
Comment at: llvm/lib/IR/ChangeReporters.cpp:858
+
+CFGDotDiff::CFGDotDiff(StringRef Title, const BlockSuccessorsFuncData &Before,
+                       const BlockSuccessorsFuncData &After, StringRef Dir) {
----------------
"Before" and "After" can be more readble. 


================
Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:546
+          "{ content.style.display = \"none\"; } else { content.style.display"
+          "= \"block\"; } }); }</script></body></html>\n";
+  HTML.flush();
----------------
Please format the string in a readble way. Please use indentation and newlines just like you'd write in a HTML file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87202