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

[Hans Wennborg]

Thanks for the review!

================
Comment at: test/Driver/parse-progname.c:1
@@ +1,2 @@
+// REQUIRES: shell, arm-registered-target
+
----------------
dblaikie wrote:
> 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.
I hope it's enough. I was inspired by some other test that required shell and seems to successfully copy stuff around and use sed.

In an msys shell, there is ln for example, though I don't know if it is faking it or uses some kind of magic.

================
Comment at: tools/driver/driver.cpp:209
@@ +208,3 @@
+      {"clang",       nullptr},
+      {"clang++",     "--driver-mode=g++"},
+      {"clang-c++",   "--driver-mode=g++"},
----------------
dblaikie wrote:
> possible cleanup: could remove the duplicated "--driver-mode=" from these strings & just build it into the use of these strings instead.
I'd rather skip on this, since it will require allocating storage for the string concatenation somewhere, so making this code simpler will make the code that adds the flag to the arguments less straight-forward.

================
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)
----------------
dblaikie wrote:
> 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)
Yes, that's much nicer. Done.

================
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))
----------------
dblaikie wrote:
> 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.
I think my loop is easier to read, but I'm pretty old-fashioned :)

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

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

================
Comment at: tools/driver/driver.cpp:249
@@ +248,3 @@
+  if (SuffixIndex == -1) {
+    // Try again after stripping any trailing version number.
+    while (!ProgNameRef.empty() &&
----------------
dblaikie wrote:
> 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?)
I'm hesitant to change the existing suffix-matching logic too much. clang++-my-branch won't work, but it didn't work before my patch either.

Also it wouldn't be completely straight-forward: cl is a substring of clang, so we'd have to be somewhat smart, and with a target prefix mixed in... hmm.

================
Comment at: tools/driver/driver.cpp:284
@@ +283,3 @@
+      const char *arr[] = { "-target", GetStableCStr(SavedStrings, Prefix) };
+      ArgVector.insert(it, arr, arr + 2);
+    }
----------------
dblaikie wrote:
> 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.
Yes, the std::begin and end were much nicer. Done.

http://reviews.llvm.org/D5833