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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay accepted this revision.
MaskRay added a comment.
This revision is now accepted and ready to land.

An alternative way organizing MRI tests is to place them into regular-to-thin-archive.test and thin-to-regular-archive.test , but a dedicated test file for MRI & Thin looks fine, too.



================
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
----------------
My personal feeling is that placing `rm, split-file, mkdir` on one line is fine (many tests do this). It's still clear.


================
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:
> 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.


================
Comment at: llvm/test/tools/llvm-ar/mri-thin-archive.test:10
 
-RUN: cd %t && llvm-ar rTc addlib/addlib.ar addlib/elf.o
+# RUN:llvm-ar rTc addlib/thin.ar addlib/elf.o
+# RUN:llvm-ar rc addlib/regular.ar addlib/elf.o
----------------
ditto below


================
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();
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128067