[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:2523-2526
+  std::vector<VersionPrinterTy> ExtraPrinters;
+
 public:
   void print() {
----------------
fpetrogalli wrote:
> There might be tools that rely on having the `ExtraPrinter` mechanisms available from `struct CommandLineCommonOptions`.  Would it be possible to just pass the extra printer here as a an argument of `print()`, and leave the `ExtraPrinters` mechanism inside  ` CommandLineCommonOptions`?
> 
> 
I will do this, though I think this is abnormal, as it is not LLVM's responsibility to not break downstream-only code.


================
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:
> 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.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137837