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

[Francesco Petrogalli via Phabricator via llvm-commits]

fpetrogalli added inline comments.


================
Comment at: llvm/lib/Support/CommandLine.cpp:2523-2526
+  std::vector<VersionPrinterTy> ExtraPrinters;
+
 public:
   void print() {
----------------
lenary wrote:
> 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.
I am happy with this answer, @lenary . It could be that I am wrong. This, together with the extra bit of "it is not LLVM's responsibility to not break downstream-only code", makes me happy with your current solution. I'l leave it up to you to decide what to do.


================
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)
----------------
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?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137837