[PATCH] Driver: support detecting driver mode when clang has a version suffix without dash (PR21094)

[David Blaikie]

Just some thoughts - mostly quite optional.

================
Comment at: test/Driver/parse-progname.c:1
@@ +1,2 @@
+// REQUIRES: shell, arm-registered-target
+
----------------
Yay tests!

Is "REQUIRES: shell" sufficient to get ln? I would wonder if we'd get a shell on Windows (mingw, perhaps) where symlinks weren't possible - but I guess maybe mingw fakes it somehow.

================
Comment at: tools/driver/driver.cpp:209
@@ +208,3 @@
+      {"clang",       nullptr},
+      {"clang++",     "--driver-mode=g++"},
+      {"clang-c++",   "--driver-mode=g++"},
----------------
possible cleanup: could remove the duplicated "--driver-mode=" from these strings & just build it into the use of these strings instead.

================
Comment at: tools/driver/driver.cpp:222
@@ +221,3 @@
+
+static int FindDriverSuffix(StringRef ProgName) {
+  for (size_t i = 0; i < llvm::array_lengthof(DriverSuffixes); ++i)
----------------
The array itself could be moved inside this function if it returned a const DriverSuffix*, which seems like a nice API for it. (null if not found, presumably)

================
Comment at: tools/driver/driver.cpp:223
@@ +222,3 @@
+static int FindDriverSuffix(StringRef ProgName) {
+  for (size_t i = 0; i < llvm::array_lengthof(DriverSuffixes); ++i)
+    if (ProgName.endswith(DriverSuffixes[i].Suffix))
----------------
I still rather like my std::find_if (& it'd be simpler once the struct had a name), but I get that it's not everyone's cup of tea.

================
Comment at: tools/driver/driver.cpp:239
@@ -236,2 +238,3 @@
+
   std::string ProgName(llvm::sys::path::stem(ArgVector[0]));
 #ifdef LLVM_ON_WIN32
----------------
Prefer copy/move initialization over direct initialization (makes it clear to the reader that no explicit conversions are happening here)?

================
Comment at: tools/driver/driver.cpp:245
@@ -242,1 +244,3 @@
+
   StringRef ProgNameRef(ProgName);
+  int SuffixIndex = FindDriverSuffix(ProgNameRef);
----------------
Prefer copy/move initialization over direct initialization (makes it clear to the reader that no explicit conversions are happening here)?

================
Comment at: tools/driver/driver.cpp:249
@@ +248,3 @@
+  if (SuffixIndex == -1) {
+    // Try again after stripping any trailing version number.
+    while (!ProgNameRef.empty() &&
----------------
this seems like a bit of a special case (& rather repetitious) - could we not just have a single call to FindDriverSuffix search for an arbitrary substring rather than a suffix?

What if someone has clang++-my-branch ? (perhaps not very common, I suppose - but still, a substring search seems simpler and more accepting - would it go wrong somewhere?)

================
Comment at: tools/driver/driver.cpp:284
@@ +283,3 @@
+      const char *arr[] = { "-target", GetStableCStr(SavedStrings, Prefix) };
+      ArgVector.insert(it, arr, arr + 2);
+    }
----------------
Hardcoding the size of the array (while obvious in the previous line) isn't ideal - the begin/end formulation I committed or the prior array_lengthof use, seem preferable.

http://reviews.llvm.org/D5833