[PATCH] Add Multilib selection machinery to Clang

[Simon Atanasyan]

================
Comment at: include/clang/Driver/Multilib.h:72
@@ +71,3 @@
+
+  bool operator== (Multilib &Other) const;
+};
----------------
- 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.

================
Comment at: include/clang/Driver/Multilib.h:36
@@ +35,3 @@
+  Multilib();
+  Multilib(std::string GCCSuffix);
+  Multilib(std::string GCCSuffix, std::string OSSuffix);
----------------
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`?

================
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"
----------------
Maybe add a `FIXME` comment here to attract attention,

================
Comment at: lib/Driver/Multilib.cpp:1
@@ +1,2 @@
+//===--- Multilib.cpp - Multilib Implementation ---------------------------===//
+//
----------------
We need test cases cover the `Multilib` functionality.

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

================
Comment at: lib/Driver/ToolChains.cpp:1563
@@ +1562,3 @@
+    Multilibs.combineWith(AndroidMipsMultilibs);
+  else if (DebianMipsMultilibs.size() == 3) {
+    Multilibs.combineWith(DebianMipsMultilibs);
----------------
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?

================
Comment at: lib/Driver/ArgParse.h:1
@@ +1,2 @@
+//===--- ArgParse.h - Argument Parsing Utilities ----------------*- C++ -*-===//
+//
----------------
Is it really necessary to move all these functions to the separate h/cpp files in this commit?


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