[PATCH] D69665: [llvm-ar] Fix llvm-ar response file reading on Windows

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:84
+  --rsp-quoting         - quoting style for response files
+    =default            -   default
+    =posix              -   posix
----------------
clang nor lld support --rsp-quoting=default. If you pass `--rsp-quoting=default` to clang, it looks like it is accepted, but the truth is that clang ignores an `--rsp-quoting` if it is passed with an unknown argument (you can try `--rsp-quoting=foo`). So, for consistency, maybe you shouldn't add that one to llvm-ar?


================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:399
     default:
-      badUsage(std::string("unknown option ") + Options[i]);
+      badUsage(std::string("unknown option ") + KeyLetters[i]);
     }
----------------
Could you revert this naming change? It makes code diff a bit hard to follow.


================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1100
 
+using ArgVector = SmallVector<const char *, 0>;
+
----------------
I don't think this `using` improves code readability.


================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1138
+  for (ArgVector::iterator ArgIt = Argv.begin(); ArgIt != Argv.end(); ++ArgIt) {
+    if (const char *Match =
+            matchFlagWithArg("rsp-quoting", ArgIt, Argv)) {
----------------
It seems that this code can be improved in terms of readability, like this:

  for (StringRef Arg : Argv) {
    if (Arg == "-rsp-quoting=posix" || Arg == "--rsp-quoting=posix")
      Ret = cl::TokenizeGNUCommandLine;
    else if (Arg == "-rsp-quoting=windows" || Arg == "--rsp-quoting=windows")
      Ret = cl::TokenizeWindowsCommandLine;
  }

This is I believe basically what we do in the clang driver.


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

https://reviews.llvm.org/D69665