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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
