[PATCH] D91890: Introduce -print-changed=[diff | diff-quiet] which show changes in patch-like format

[Arthur Eubanks via Phabricator via llvm-commits]

aeubanks added a comment.

mostly looks good, a couple nits



================
Comment at: llvm/include/llvm/Passes/StandardInstrumentations.h:303
+  // \p Before and \p After.  The order is based on the order in \p After
+  // with ones that are only in \p Before intersperced based on where they
+  // occur in \p Before.  This is used to present the output in an order
----------------



================
Comment at: llvm/include/llvm/Passes/StandardInstrumentations.h:308
+  report(const OrderedChangedData &Before, const OrderedChangedData &After,
+         std::function<void(const IRData *, const IRData *)> HandlePair);
+
----------------
`function_ref` is much lighter weight.


================
Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:71-72
 // value of quiet will only report when the IR changes, suppressing
-// all other messages, including the initial IR.
-enum ChangePrinter { NoChangePrinter, PrintChangedVerbose, PrintChangedQuiet };
+// all other messages, including the initial IR.  The values of diff and
+// diff-quiet will present the changes in a form similar to a patch, in
+// either verbose or quiet mode, respectively.  The lines that are removed
----------------
The wording here tripped me up, perhaps `The values "diff" and "diff-quiet" will...`.


================
Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:80
+//
+enum ChangePrinter {
+  NoChangePrinter,
----------------
`enum class`?


================
Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:590
+  // Print out the IRData in the after order, with before ones interspersed
+  // appropriately (ie, somewhere near where the were in the before list).
+  // Start at the beginning of both lists.  Loop through the
----------------



================
Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:1114-1161
+InLineChangePrinter::~InLineChangePrinter() {}
+
+void InLineChangePrinter::generateIRRepresentation(Any IR, StringRef PassID,
+                                                   ChangedIRData &D) {
+  ChangedIRComparer::analyzeIR(IR, D);
+}
+
----------------
can you move these above the `StandardInstrumentations` code?


================
Comment at: llvm/test/Other/ChangePrinters/lit.local.cfg:5
+def have_needed_diff_support():
+    if not os.path.exists('/usr/bin/diff'):
+        return False
----------------
I'm wondering if we should just detect any `diff` in the `PATH`, but maybe just `/usr/bin/diff` is fine, idk


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

https://reviews.llvm.org/D91890