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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/test/tools/llvm-ar/response-utf8.test:8
+## Work-around diff not working with non-ascii filenames on windows.
+RUN: env LANG=en_US.UTF-8 %python -c "assert open(ur'%t-\U000000A3.a', 'rb').read() == open(r'%t.a', 'rb').read()"
----------------
thopre wrote:
> Could you try using the same approach as in https://github.com/llvm/llvm-project/blob/c3eded068c64bfe614d25359927a2917ff8e4a35/llvm/test/tools/llvm-ar/mri-nonascii.test#L19 ?
@thopre Hi, have you tried fixing the macOS (or Windows) problem?


================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1100
 
+typedef SmallVector<const char *, 0> ArgVector;
+
----------------
Prefer using in new code. It can be used in an alias template and thus improve consistency with other constructs of the language.


================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1103
+static StringRef removeHyphens(StringRef Arg) {
+  if (Arg[0] != '-')
+    return StringRef();
----------------
I feel that we can get rid of some ` if (Arg.empty())` checks if `matchFlagWithArg` itself recognizes one or two dash, then removeHyphens may probably be unnecessary.


================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1127
+
+static cl::TokenizerCallback getRspQuoting(ArgVector &Argv) {
+  cl::TokenizerCallback Default =
----------------
Argv is passed as a non-const reference but it is not actually mutated in the function as I expected. Should getRspQuoting remove --rsp-quoting so that on L1204 we don't need `    if (matchFlagWithArg("rsp-quoting", Arg, ArgIt, Argv.end())...` ?


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

https://reviews.llvm.org/D69665