Allow VersionPrinter to print to arbitrary raw_ostreams

[dim]

Summary

I would like to add printing of registered targets to clang's version
information. For this to work correctly, the VersionPrinter logic in
CommandLine.cpp should support printing to arbitrary raw_ostreams,
instead of always defaulting to outs().

Add a raw_ostream& parameter to the function pointer type used for
VersionPrinter, and while doing so, introduce a typedef for convenience.

Note that VersionPrinter::print() will still default to using outs(),
the clang part will necessarily go into a separate review.

[mehdi_amini]

include/llvm/Support/CommandLine.h

69

Using a std::function (or maybe llvm::function_ref) would be allowing for more rich object to be passed in (i.e. a client that may want state to be passed around).

[dim]

include/llvm/Support/CommandLine.h

69

Maybe, but how would VersionPrinter::print or VersionPrinter::operator= know how to call that function object, and which parameters to pass?

In any case it looks like llvm::function_ref also doesn't support variable function types, you'd still need to settle on function_ref<void(raw_ostream &)> as 'the' type for version printing functions. With std::function it seems to be the same.

The only advantage would maybe be the ownership of the function object, but I'm not sure if that is a big deal breaker in this case. It's just for printing a version field. :)

[dim]

Alternatively, as a lower impact change, I could just add the code from printRegisteredTargetsForVersion to tools/clang/lib/Driver/Driver.cpp, but it's a little ugly...

[mehdi_amini]

include/llvm/Support/CommandLine.h

69

It is not about the function signature, it is about being able to have a callback with *state*.

i.e. what people do in C is adding a void * parameter to the callback signature and have the caller passing it in alongside the callback, C++ has std::function for this.

[dim]

include/llvm/Support/CommandLine.h

69

Sure, but in this case the callback signature has a raw_ostream &. Did you mean you wanted the following:

typedef void (*VersionPrintTy)(std::function<whatever> &);

? I'm not sure how that would be handy, though.

[mehdi_amini]

include/llvm/Support/CommandLine.h

69

No I meant something like:

typedef std::function<void(raw_ostream &>)> VersionPrintTy;

[dim]

Use a std::function object instead of a plain function pointer.

[mehdi_amini]

LGTM, thanks.

include/llvm/Support/CommandLine.h

 
 //===----------------------------------------------------------------------===//
 // ParseEnvironmentOptions - Environment variable option processing alternate
 //                           entry point.
 //
 void ParseEnvironmentOptions(const char *progName, const char *envvar,
                              const char *Overview = "");
 
+// Function pointer type for printing version information.
+typedef std::function<void(raw_ostream &)> VersionPrinterTy;
+
 ///===---------------------------------------------------------------------===//
 /// SetVersionPrinter - Override the default (LLVM specific) version printer
 ///                     used to print out the version when --version is given
 ///                     on the command line. This allows other systems using the
 ///                     CommandLine utilities to print their own version string.
-void SetVersionPrinter(void (*func)());
+void SetVersionPrinter(VersionPrinterTy func);
 
 ///===---------------------------------------------------------------------===//
 /// AddExtraVersionPrinter - Add an extra printer to use in addition to the
 ///                          default one. This can be called multiple times,
 ///                          and each time it adds a new function to the list
 ///                          which will be called after the basic LLVM version
 ///                          printing is complete. Each can then add additional
 ///                          information specific to the tool.
-void AddExtraVersionPrinter(void (*func)());
+void AddExtraVersionPrinter(VersionPrinterTy func);
 
 // PrintOptionValues - Print option values.
 // With -print-options print the difference between option values and defaults.
 // With -print-all-options print all option values.
 // (Currently not perfect, but best-effort.)
 void PrintOptionValues();
 
 // Forward declaration - AddLiteralOption needs to be up here to make gcc happy.

include/llvm/Support/TargetRegistry.h

const Target &operator*() const {
       return *Current;
     }
 
     const Target *operator->() const { return &operator*(); }
   };
 
   /// printRegisteredTargetsForVersion - Print the registered targets
   /// appropriately for inclusion in a tool's version output.
-  static void printRegisteredTargetsForVersion();
+  static void printRegisteredTargetsForVersion(raw_ostream &OS);
 
   /// @name Registry Access
   /// @{
 
   static iterator_range<iterator> targets();
 
   /// lookupTarget - Lookup a target based on a target triple.
   ///

lib/Support/CommandLine.cpp

void CommandLineParser::printOptionValues() {
   size_t MaxArgLen = 0;
   for (size_t i = 0, e = Opts.size(); i != e; ++i)
     MaxArgLen = std::max(MaxArgLen, Opts[i].second->getOptionWidth());
 
   for (size_t i = 0, e = Opts.size(); i != e; ++i)
     Opts[i].second->printOptionValue(MaxArgLen, PrintAllOptions);
 }
 
-static void (*OverrideVersionPrinter)() = nullptr;
+static VersionPrinterTy OverrideVersionPrinter = nullptr;
 
-static std::vector<void (*)()> *ExtraVersionPrinters = nullptr;
+static std::vector<VersionPrinterTy> *ExtraVersionPrinters = nullptr;
 
 namespace {
 class VersionPrinter {
 public:
   void print() {
     raw_ostream &OS = outs();
 #ifdef PACKAGE_VENDOR
     OS << PACKAGE_VENDOR << " ";

 #endif
     OS << '\n';
   }
   void operator=(bool OptionWasSpecified) {
     if (!OptionWasSpecified)
       return;
 
     if (OverrideVersionPrinter != nullptr) {
-      (*OverrideVersionPrinter)();
+      OverrideVersionPrinter(outs());
       exit(0);
     }
     print();
 
     // Iterate over any registered extra printers and call them to add further
     // information.
     if (ExtraVersionPrinters != nullptr) {
       outs() << '\n';
-      for (std::vector<void (*)()>::iterator I = ExtraVersionPrinters->begin(),
-                                             E = ExtraVersionPrinters->end();
-           I != E; ++I)
-        (*I)();
+      for (auto I : *ExtraVersionPrinters)
+        I(outs());
     }
 
     exit(0);
   }
 };
 } // End anonymous namespace
 
 // Define the --version option that prints out the LLVM version for the tool
else if (Hidden && !Categorized)
     UncategorizedHiddenPrinter = true;
   else
     CategorizedHiddenPrinter = true;
 }
 
 /// Utility function for printing version number.
 void cl::PrintVersionMessage() { VersionPrinterInstance.print(); }
 
-void cl::SetVersionPrinter(void (*func)()) { OverrideVersionPrinter = func; }
+void cl::SetVersionPrinter(VersionPrinterTy func) { OverrideVersionPrinter = func; }
 
-void cl::AddExtraVersionPrinter(void (*func)()) {
+void cl::AddExtraVersionPrinter(VersionPrinterTy func) {
   if (!ExtraVersionPrinters)
-    ExtraVersionPrinters = new std::vector<void (*)()>;
+    ExtraVersionPrinters = new std::vector<VersionPrinterTy>;
 
   ExtraVersionPrinters->push_back(func);
 }
 
 StringMap<Option *> &cl::getRegisteredOptions(SubCommand &Sub) {
   auto &Subs = GlobalParser->RegisteredSubCommands;
   (void)Subs;
   assert(is_contained(Subs, &Sub));

lib/Support/TargetRegistry.cpp

void TargetRegistry::RegisterTarget(Target &T,
   T.HasJIT = HasJIT;
 }
 
 static int TargetArraySortFn(const std::pair<StringRef, const Target *> *LHS,
                              const std::pair<StringRef, const Target *> *RHS) {
   return LHS->first.compare(RHS->first);
 }
 
-void TargetRegistry::printRegisteredTargetsForVersion() {
+void TargetRegistry::printRegisteredTargetsForVersion(raw_ostream &OS) {
   std::vector<std::pair<StringRef, const Target*> > Targets;
   size_t Width = 0;
   for (const auto &T : TargetRegistry::targets()) {
     Targets.push_back(std::make_pair(T.getName(), &T));
     Width = std::max(Width, Targets.back().first.size());
   }
   array_pod_sort(Targets.begin(), Targets.end(), TargetArraySortFn);
 
-  raw_ostream &OS = outs();
   OS << "  Registered Targets:\n";
   for (unsigned i = 0, e = Targets.size(); i != e; ++i) {
     OS << "    " << Targets[i].first;
     OS.indent(Width - Targets[i].first.size()) << " - "
       << Targets[i].second->getShortDescription() << '\n';
   }
   if (Targets.empty())
     OS << "    (none)\n";
 }