[PATCH] D78346: Fix Windows command line bug when last token in response file is ""

Tue May 19 09:17:03 PDT 2020

amccarth added a comment.

I think the fix looks good, but I'd like to see a bit more in the test.

Thanks for taking on this bug.

================
Comment at: llvm/unittests/Support/CommandLineTest.cpp:258
+      "a\\b c\\\\d e\\\\\"f g\" h\\\"i j\\\\\\\"k \"lmn\" o pqr "
+      "\"st \\\"u\" \\v \"\"";
+  const char *const Output[] = {"a\\b",   "c\\\\d", "e\\f g", "h\"i",
----------------
It would make the test easier to read if we could use raw string literals for these kinds of inputs.  Does LLVM style permit that?

================
Comment at: llvm/unittests/Support/CommandLineTest.cpp:261
+                                "j\\\"k", "lmn",    "o",      "pqr",
+                                "st \"u", "\\v",    ""};
   testCommandLineTokenizer(cl::TokenizeWindowsCommandLine, Input, Output,
----------------
I'm mildly concerned that in the future someone may come in and append another argument to the end in order to test some other case, and thus invalidate this test.

Can you add a comment that this test is (among other things) explicitly testing the case of an empty quoted string at the end of the command?  Or maybe even make this a separate test for specifically that (e.g., `TokenizeWindowsCommandLineQuotedLastArgument`).

Are there other cases we should test to make sure this doesn't change things?  What if the last argument is `"` or `"""` or `""""`?  What if there's white space after the last quotation mark?  A quick look at the code suggests these might all work with this change, but it looks like they'd hit some other code paths.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78346/new/

https://reviews.llvm.org/D78346