[PATCH] D137837: [Support] Move Target/CPU Printing out of CommandLine

[Sam Elliott via Phabricator via llvm-commits]

lenary added inline comments.


================
Comment at: llvm/lib/Support/CommandLine.cpp:2542-2549
     OS << '\n';
+
+    // Iterate over any registered extra printers and call them to add further
+    // information.
+    if (!ExtraPrinters.empty()) {
+      outs() << '\n';
+      for (const auto &I : ExtraPrinters)
----------------
fpetrogalli wrote:
> lenary wrote:
> > fpetrogalli wrote:
> > > I think you have modified the output here. I believe that now the tools will print 2 empty lines right after `" with assertions"`, while before it was printing only one empty line. Maybe it is not important, but I suspect that there might be some tools that parse the output printed by this function. Better to make sure we can guarantee the same output. 
> > I'm concerned about this suggestion, but I'm going to do it.
> > 
> > Again, nothing in LLVM Upstream tests the exact format, or expects it to be stable. There is one test, (modified below, but I'm going to revert the change), which checks for the target-detected output, and that's it.
> > 
> > It's also going to be a big issue if someone is checking the version output of a utility that will no longer do target detection after this change (because, this is the main functional change in this patch). Again, if there was testing of this behaviour I'd be happier.
> > 
> Well, it is not a bit change to move line 2542 `OS << '\n';` after this is statement? :)
> 
> (Or am I missing something?)
That suggestion still changes the output, it's just the extra newline is now trailing :)

Sorry it took me a while to get back on this, rebasing and addressing comments on the whole sequence means there's quite a slow turnaround on updates to these patches


================
Comment at: llvm/tools/llvm-lto/llvm-lto.cpp:49
 #include "llvm/Support/WithColor.h"
+#include "llvm/Support/raw_ostream.h"
 #include "llvm/Target/TargetOptions.h"
----------------
Will undo this irrelevant change when I apply the patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137837