[PATCH] D91890: Introduce -print-changes which show changes in patch-like format

[Arthur Eubanks via Phabricator via llvm-commits]

aeubanks added inline comments.


================
Comment at: llvm/include/llvm/Passes/StandardInstrumentations.h:274
+// before and after the pass to determine if it was changed by a pass.
+template <typename BlockDataType> class BlockDataTemplate {
+public:
----------------
Apologies if I'm forgetting things you've said in the past, but what are the other future use cases of this? I'd prefer less templatization when possible.


================
Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:94
+static cl::opt<bool> PrintChanges(
+    "print-changes", cl::Hidden, cl::init(false),
+    cl::desc("Print in-line differences of changes made by a pass"));
----------------
what about "print-changed-diff"? "print-changes" seems a bit too closed to "print-changed"


================
Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:103
+// for any failures instead of the diff.
+std::string doLinuxDiff(StringRef Before, StringRef After,
+                        StringRef OldLineFormat, StringRef NewLineFormat,
----------------
probably shouldn't have `Linux` in the name, this could work on other platforms.


================
Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:139
+  std::array<char, 128> Buffer;
+  FILE *Pipe = popen(DiffCall.c_str(), "r");
+  if (!Pipe)
----------------
maybe this should be using the APIs in Program.h instead? `popen` seems a bit too C-like.


================
Comment at: llvm/test/Other/ChangePrinters/lit.local.cfg:13-16
+    if not '-line-format' in ld_out:
+        return False
+
+    return True
----------------
`return '-line-format' in ld_out`


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

https://reviews.llvm.org/D91890