[PATCH] D128067: [llvm-ar] Fix MRI ADDLIB command when used with thin archives

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-ar/mri-thin-archive.test:1
-RUN: rm -rf %t && mkdir -p %t/addlib/
+# RUN: rm -rf %t
+# RUN: split-file %s %t
----------------
MaskRay wrote:
> MaskRay wrote:
> > My personal feeling is that placing `rm, split-file, mkdir` on one line is fine (many tests do this). It's still clear.
> `# UNSUPPORTED: system-aix`
> 
> AIX doesn't use `<arch>`, and I am unsure how thin archives work for them.
Alternatively, use a --format option in the llvm-ar invocations, but it's probably simpler to use the `# UNSUPPORTED` as @MaskRay suggests.


================
Comment at: llvm/test/tools/llvm-ar/mri-thin-archive.test:14
+# RUN: llvm-ar -M < thin.script
+# RUN: FileCheck -input-file=archive.ar %s
 
----------------
I'd give this check an explicit prefix, rather than relying on the default, since you have other prefixes elsewhere.


================
Comment at: llvm/test/tools/llvm-ar/mri-thin-archive.test:19
+# CHECK-NEXT: addlib/elf.o/
+# CHECK-NOT: delete.o
 
----------------
This will only show that `delete.o` is not the //last// member of the archive. It would probably be more advisable to use FileCheck `--implicit-check-not=delete.o` (I know that this was here before, but since you're basically rewriting the test, you might as well improve it whilst here).


================
Comment at: llvm/test/tools/llvm-ar/mri-thin-archive.test:25
+# REGULAR: !<arch>
+# REGULAR: elf.o
+
----------------
What is the purpose of this line? All it does is show that `elf.o` appears somewhere in the archive, and doesn't confirm whether or not a) there are any other members and b) whether the name is a full path (because FileCheck looks for substrings unless told to do otherwise).


================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1068
+        if (Thin && !Lib.isThin())
+          fail("cannot add a regular archives contents to a thin archive");
         Error Err = Error::success();
----------------
MaskRay wrote:
> grammar mistakes in "a regular archives contents"
> 
> I'd suggest `cannot merge a regular archive into a thin archive`, but happy to use one @jhenderson prefers.
I'm not a fan of the word "merge" here, because it has implications around the old regular archive no longer existing. I think the original is closer to being correct, but missing an apostrophe: "cannot add a regular archive's contents to a thin archive".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128067