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

[Owen Reynolds via Phabricator via llvm-commits]

gbreynoo added inline comments.


================
Comment at: llvm/test/tools/llvm-ar/mri-thin-archive.test:25
+# REGULAR: !<arch>
+# REGULAR: elf.o
+
----------------
jhenderson wrote:
> 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).
This was just to check the archive was regular, I've removed the unneeded check for `elf.o`


================
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();
----------------
jhenderson wrote:
> 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".
I went for adding the apostrophe.


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

https://reviews.llvm.org/D128067