[PATCH] [Support] Beef up and expose the response file parsing in llvm::cl

[Hans Wennborg]

  This looks great, I just have minor comments.


================
Comment at: include/llvm/Support/CommandLine.h:1787
@@ +1786,3 @@
+typedef void TokenizerCallback(StringRef Source, StringSaver &Saver,
+                               SmallVectorImpl<const char *> &NewArgv);
+
----------------
I think it's more common to typedef a callback type as pointer-to-function instead of just function.

================
Comment at: include/llvm/Support/CommandLine.h:1796
@@ +1795,3 @@
+/// \param [in,out] Argv Command line into which to expand response files.
+void ExpandResponseFiles(StringSaver &Saver, TokenizerCallback *Tokenize,
+                         SmallVectorImpl<const char *> &Argv);
----------------
if TokenizerCallback is typedeffed to ptr-to-function, Tokenize would be passed by value (well, the value would be a pointer), which I think is more common for callbacks. Also, maybe rename the parameter to "Tokenizer"?

================
Comment at: lib/Support/CommandLine.cpp:453
@@ +452,3 @@
+                                SmallVectorImpl<const char *> &NewArgv) {
+  SmallString<128> Tok;
+  for (size_t I = 0, E = Src.size(); I != E; ++I) {
----------------
no big deal, but we might as well call this Token?

================
Comment at: lib/Support/CommandLine.cpp:465
@@ +464,3 @@
+    if (I + 1 < E && Src[I] == '\\' && isGNUSpecial(Src[I + 1])) {
+      ++I;  // Consume the escape.
+      Tok.push_back(Src[I]);
----------------
isn't it more "skip" than consume?

================
Comment at: lib/Support/CommandLine.cpp:538
@@ +537,3 @@
+    const char *Arg = Argv[I];
+    if (Arg[0] != '@' || RspFiles > 20) {
+      ++I;
----------------
should we just break out of the loop when RspFiles hits 20?

================
Comment at: unittests/Support/CommandLineTest.cpp:129
@@ +128,3 @@
+TEST(CommandLineTest, TokenizeGNUCommandLine) {
+  const char *Input = "foo\\ bar \"foo bar\" \'foo bar\' 'foo\\\\bar'";
+  const char *const Output[] = { "foo bar", "foo bar", "foo bar", "foo\\bar" };
----------------
maybe add a test to check that a token doesn't have to end after a quote, i.e. foo"bar"baz is just one token: foobarbaz

================
Comment at: unittests/Support/CommandLineTest.cpp:129
@@ +128,3 @@
+TEST(CommandLineTest, TokenizeGNUCommandLine) {
+  const char *Input = "foo\\ bar \"foo bar\" \'foo bar\' 'foo\\\\bar'";
+  const char *const Output[] = { "foo bar", "foo bar", "foo bar", "foo\\bar" };
----------------
Hans Wennborg wrote:
> maybe add a test to check that a token doesn't have to end after a quote, i.e. foo"bar"baz is just one token: foobarbaz
and maybe stick a windows file path in there to make sure we don't do anything crazy with the backslashes


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