[PATCH] D128055: [llvm-ar] Improve MRI script CREATE command handling

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-ar/mri-addlib.test:28
+
+## ADDLIB with missing file
+# RUN: not llvm-ar -M < missing.mri 2>&1 | FileCheck --check-prefix=MISSING %s
----------------
Nit: here and below, missing full stop at end of sentence.


================
Comment at: llvm/test/tools/llvm-ar/mri-addlib.test:32
+
+# MISSING: error: script line 2: could not open library {{.*}} no such file or directory
+
----------------
I believe you should use one of the %errc substitutions (I think `%errc_NOENT` if memory serves correctly, but look it up!)

That way, the message is platform independent. 


================
Comment at: llvm/test/tools/llvm-ar/mri-addlib.test:42-43
+
+# FILES2: f.o
+# FILES2: f.o
+
----------------
`FILES2-COUNT-2` along the same lines as above, assuming they are in adjacent lines in the output.


================
Comment at: llvm/test/tools/llvm-ar/mri-addmod.test:1
 ## Test the ADDMOD MRI command.
 
----------------
Same comments from me apply to this test.


================
Comment at: llvm/test/tools/llvm-ar/mri-create.test:10
+
+## Test use of CREATE with no archive name
+# RUN: not llvm-ar -M < none.mri 2>&1 | FileCheck --check-prefix=NONE %s
----------------
Nit: missing full stops.


================
Comment at: llvm/test/tools/llvm-ar/mri-create.test:29
+
+## Test duplicate use of CREATE
+# RUN: not llvm-ar -M < create2.mri 2>&1 | FileCheck --check-prefix=MULTIPLE %s
----------------
Is `CREATE` -> `SAVE` -> `CREATE` supported? If so, do you need test coverage for that too?


================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1066
+      if (!Create)
+        fail("no open output archive");
       object::Archive &Lib = readLibrary(Rest);
----------------
MaskRay wrote:
> output archive is not opened?
 I prefer "no output archive has been opened"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128055