[PATCH] Add Multilib selection machinery to Clang

[Jon Roelofs]

================
Comment at: lib/Driver/ArgParse.h:1
@@ +1,2 @@
+//===--- ArgParse.h - Argument Parsing Utilities ----------------*- C++ -*-===//
+//
----------------
Simon Atanasyan wrote:
> Is it really necessary to move all these functions to the separate h/cpp files in this commit?
A few of these would end up being duplicated if I left them as static functions. Would it be better to do this in a separate commit?  Do you have a better suggestion for the name of this file? (I don't really like 'ArgParse', but it's the best I could come up with).

================
Comment at: lib/Driver/Driver.cpp:778
@@ -771,19 +777,3 @@
 
-  // FIXME: What is the difference between print-multi-directory and
-  // print-multi-os-directory?
-  if (C.getArgs().hasArg(options::OPT_print_multi_directory) ||
-      C.getArgs().hasArg(options::OPT_print_multi_os_directory)) {
-    switch (C.getDefaultToolChain().getTriple().getArch()) {
-    default:
-    case llvm::Triple::x86:
-    case llvm::Triple::ppc:
-      llvm::outs() << "." << "\n";
-      break;
-
-    case llvm::Triple::x86_64:
-      llvm::outs() << "x86_64" << "\n";
-      break;
-
-    case llvm::Triple::ppc64:
-      llvm::outs() << "ppc64" << "\n";
-      break;
+  if (C.getArgs().hasArg(options::OPT_print_multi_os_directory)) {
+    // This should print out "lib/../lib", "lib/../lib64", or "lib/../lib32"
----------------
Simon Atanasyan wrote:
> Maybe add a `FIXME` comment here to attract attention,
Ok

================
Comment at: lib/Driver/Multilib.cpp:33-47
@@ +32,17 @@
+
+Multilib::Multilib() {
+}
+
+Multilib::Multilib(std::string GCCSuffix)
+  : GCCSuffix(GCCSuffix) {
+}
+
+Multilib::Multilib(std::string GCCSuffix, std::string OSSuffix)
+  : GCCSuffix(GCCSuffix), OSSuffix(OSSuffix) {
+}
+
+Multilib::Multilib(std::string GCCSuffix, std::string OSSuffix,
+    std::string IncludeSuffix)
+  : GCCSuffix(GCCSuffix), OSSuffix(OSSuffix), IncludeSuffix(IncludeSuffix) {
+}
+
----------------
Sean Silva wrote:
> Dmitri Gribenko wrote:
> > There is no reason to put these constructors out-of-line.
> Could default arguments reduce the boilerplate here?
Ok, I'll combine them and put the declaration inline with the class.

================
Comment at: lib/Driver/Multilib.cpp:175
@@ +174,3 @@
+static Multilib compose(const Multilib &Base, const Multilib &New) {
+  Multilib Composed(Base.gccSuffix() + New.gccSuffix(),
+                    Base.osSuffix() + New.osSuffix(),
----------------
Simon Atanasyan wrote:
> It's potentially dangerous to join two string represent arbitrary paths. Can we trust that we never get double slashes `//` and no one slash at all at the point of joining?
Should I add assertions? Would it be better to use llvm/Support/Path.h for these suffixes?

================
Comment at: lib/Driver/ToolChains.cpp:1563
@@ +1562,3 @@
+    Multilibs.combineWith(AndroidMipsMultilibs);
+  else if (DebianMipsMultilibs.size() == 3) {
+    Multilibs.combineWith(DebianMipsMultilibs);
----------------
Simon Atanasyan wrote:
> Magical code on this line and below. Why do we compare `DebianMipsMultilibs.size()` and 3? Why do we combine `Multilibs` and `FSFMipsMultilibs` if `FSFMipsMultilibs.size() > CSMipsMultilibs.size()` etc?
Yes, this is a bit of 'Magic', but I'm not sure of a better solution. The problem here is that each of the MultilibSets overlap a little, so you get partial matches when looking at one installation and comparing it with the 'wrong' MultilibSet.  I could add a comment explaining this, or maybe you a have a suggestion for a better approach :) ?

================
Comment at: include/clang/Driver/Multilib.h:36
@@ +35,3 @@
+  Multilib();
+  Multilib(std::string GCCSuffix);
+  Multilib(std::string GCCSuffix, std::string OSSuffix);
----------------
Simon Atanasyan wrote:
> I think it's ineffective to use `std::string` as a type of a function's argument. Maybe `StringRef` or a const reference to `std::string`?
Ok. StringRef would be better.

================
Comment at: include/clang/Driver/Multilib.h:72
@@ +71,3 @@
+
+  bool operator== (Multilib &Other) const;
+};
----------------
Simon Atanasyan wrote:
> - It's better to pass `Other` by const reference.
> - Do not add a space between a function name and an open brace. You can run the `clang-format` on your patch to find and fix all formatting issues.
Oh, I didn't know I could run it on a patch... cool.

================
Comment at: lib/Driver/Multilib.cpp:1
@@ +1,2 @@
+//===--- Multilib.cpp - Multilib Implementation ---------------------------===//
+//
----------------
Simon Atanasyan wrote:
> We need test cases cover the `Multilib` functionality.
Like the ones in clang/unittests or clang/test? Both?


http://llvm-reviews.chandlerc.com/D2538