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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

> Context not available.

https://llvm.org/docs/GettingStarted.html#sending-patches

  apt install arcanist (clone libphutil + clone arcanist)
  cd llvm; arc install-certificate
  arc diff 'HEAD^'  # Make sure the commit message includes `Differential Revision: `. Update the revision with a full context



================
Comment at: llvm/test/tools/llvm-ar/response.test:24
+# RUN: not llvm-ar --rsp-quoting=foobar @%t.response1.txt 2>&1 | \
+# RUN: FileCheck  %s --check-prefix=ERROR
+# ERROR: Invalid response file quoting style foobar
----------------
If `\` has to be used, it is recommended to indent the continuation lines to make them stand out.


================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:182
 
-static std::string Options;
+static std::string KeyLetters;
 
----------------
Why `KeyLetters`? I think the ar terminology uses operations and modifiers. Collectively they can be called options.


================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1123
+  }
+  if (Arg.startswith(Expected) && Arg.size() > len && Arg[len] == '=') {
+    return Arg.data() + len + 1;
----------------
Redundant pair of braces


================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1181
+
+    if (strcmp(*ArgIt,"-M") == 0) {
+      MRI = true;
----------------
space after `,`


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

https://reviews.llvm.org/D69665