[PATCH] Teach Clang how to use response files when calling other tools

[Sean Silva]

================
Comment at: lib/Driver/Job.cpp:105
@@ +104,3 @@
+static bool ArgNeedsQuotes(const char *Str) {
+  return Str[0] == '\0' || strpbrk(Str, "\t \"&\'()*<>\\`^|") != 0;
+}
----------------
Tiny nit: Use nullptr instead of 0

================
Comment at: lib/Driver/Job.cpp:133-139
@@ +132,9 @@
+/// Check if string \p Arg is present in the list of inputs.
+static bool isInInputList(const llvm::SmallVectorImpl<const char *> &Inputs,
+                          const char *Arg) {
+  for (auto InputName : Inputs) {
+    if (strcmp(Arg, InputName) == 0)
+      return true;
+  }
+  return false;
+}
----------------
In the future, consider using std::find_if instead of writing this function http://en.cppreference.com/w/cpp/algorithm/find

I'm not sure a linear search here is acceptable though, see my comment below.

================
Comment at: lib/Driver/Job.cpp:144-150
@@ +143,9 @@
+/// file.
+static llvm::SmallVector<const char *, 128>
+buildArgvForResponseFile(const char *ResponseFlag, const char *Executable) {
+  llvm::SmallVector<const char *, 128> Argv;
+  Argv.push_back(Executable);
+  Argv.push_back(ResponseFlag);
+  return Argv;
+}
+
----------------
This seems like an unusual thing to have a helper for. The real problem seems to be that this is part of a larger block of code which is duplicated. Just pulling out this part doesn't help things: get rid of this helper function and deduplicate a larger scope.

================
Comment at: lib/Driver/Job.cpp:163
@@ +162,3 @@
+/// \p Arguments - Original arguments when calling the tool
+static llvm::SmallVector<const char *, 128>
+buildArgvForFileList(const llvm::SmallVectorImpl<const char *> &Inputs,
----------------
Returning a SmallVector by value is undesirable. in this case, I think NRVO will negate part of the issue (large sizeof), but you still have the problem that this hardcodes what SmallVector size the caller is using. I would recommend switching this to taking a non-const `SmallVectorImpl<const char *> &Out` instead of returning a SmallVector.

================
Comment at: lib/Driver/Job.cpp:172
@@ +171,3 @@
+  for (auto Arg : Arguments) {
+    if (!isInInputList(Inputs, Arg)) {
+      Argv.push_back(Arg);
----------------
isInInputList seems to be doing linear work. If Arguments is mostly Inputs, then this will go quadratic. Is that a potential problem?

================
Comment at: lib/Driver/Job.cpp:185-186
@@ +184,4 @@
+static void
+writeFileList(raw_ostream &OS,
+              const llvm::SmallVectorImpl<const char *> &Inputs) {
+  for (auto Arg : Inputs) {
----------------
Use ArrayRef here.

================
Comment at: lib/Driver/Job.cpp:187-190
@@ +186,6 @@
+              const llvm::SmallVectorImpl<const char *> &Inputs) {
+  for (auto Arg : Inputs) {
+    OS << Arg;
+    OS << '\n';
+  }
+}
----------------
Simpler:

```
for (auto Arg : Inputs)
  OS << Arg << '\n';
```

Also, for clarity, consider just saying const char * instead of auto.

================
Comment at: lib/Driver/Job.cpp:205
@@ +204,3 @@
+    ResponseFlag = Creator.getResponseFileFlag();
+    ResponseFlag += ResponseFile.data();
+    if (RespFileSup == Tool::RF_FileList)
----------------
I don't think this is correct. ResponseFile.data() isn't guaranteed to be 0 terminated (and you don't want to embed that hidden assumption). Maybe use .append(begin, end)?

================
Comment at: lib/Driver/Job.cpp:216-232
@@ -104,16 +215,19 @@
+
+  for (size_t i = 0, e = Args.size(); i < e; ++i) {
+    const char *const Arg = Args[i];
 
     if (CrashReport) {
       if (int Skip = skipArgs(Arg)) {
         i += Skip - 1;
         continue;
       }
     }
 
     OS << ' ';
     PrintArg(OS, Arg, Quote);
 
     if (CrashReport && quoteNextArg(Arg) && i + 1 < e) {
       OS << ' ';
-      PrintArg(OS, Arguments[++i], true);
+      PrintArg(OS, Args[++i], true);
     }
   }
----------------
This loop scares me. Could you clean this up in a separate patch? Or at least add comment here for now.

================
Comment at: lib/Driver/Job.cpp:235
@@ +234,3 @@
+
+  if (NeedsResponseFile && ResponseFile.data() != nullptr) {
+    OS << "\n Arguments passed via response file:\n";
----------------
This call seems duplicated, along with a lot of stuff in this if. Pull this into a helper?

================
Comment at: lib/Driver/Tools.h:309
@@ +308,3 @@
+    }
+    virtual const char *getResponseFileFlag() const override {
+      return "-filelist";
----------------
Tiny nit: Don't use `virtual` with `override`.

================
Comment at: lib/Driver/Tools.h:531-537
@@ -441,1 +530,9 @@
+
+    ResponseFileSupport getResponseFilesSupport() const override {
+      return RF_Full;
+    }
+    llvm::sys::fs::WindowsEncodingMethod
+    getResponseFileEncoding() const override {
+      return llvm::sys::fs::WEM_CurrentCodePage;
+    }
   };
----------------
This seems highly repetitive. Is there a way to reduce it?

Also, I'm not sure that it makes sense to have a method returning WindowsEncodingMethod for a netbsd tool (same goes for most tools that have no reason to run on windows).

http://reviews.llvm.org/D4897