[PATCH] D123142: [llvm-ar] Fix thin archive being wrongly converted to a full archive

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-ar/full-to-thin-archive.test:11
 
-## Test that you can add a full archive's contents to a thin archive with 'L'
-# RUN: llvm-ar -TqcL %t/thin.a %t/archive.a
-# RUN: FileCheck --check-prefixes=THIN --input-file=%t/thin.a %s
+## Test that you can add a full archive to a thin archive with 'L'
+# RUN: llvm-ar -qcL --thin %t/thin1.a %S/Inputs/b.txt %t/archive.a
----------------



================
Comment at: llvm/test/tools/llvm-ar/full-to-thin-archive.test:15
 
-THIN: !<thin>
+## Test that you can add a full archive to an existing thin archive with 'L'
+# RUN: llvm-ar -q --thin %t/thin2.a %S/Inputs/b.txt
----------------
You've used the term "full archive" here and in the comment above, but simply "archive" in other places. Be consistent, use one version. FWIW, the error message is "regular archive".


================
Comment at: llvm/test/tools/llvm-ar/full-to-thin-archive.test:24
+## Test archives do not convert to thin archives with use of 'L' and '--thin'.
+# RUN: not llvm-ar -qL --thin %t/archive.a %t/thin2.a
+# RUN: FileCheck --check-prefixes=FULL --input-file=%t/archive.a %s
----------------
I recommend checking the error message here.


================
Comment at: llvm/test/tools/llvm-ar/thin-to-full-archive.test:28
+# FULL: !<arch>
+# NOT-FULL: thin.a
+# FULL: a.txt
----------------
This isn't used anywhere. Did you mean `FULL-NOT`? If so, I'd recommend instead adding `--implicit-check-not=thin.a` to the FileCheck command, since NOT matches are order-sensitive.


================
Comment at: llvm/test/tools/llvm-ar/thin-to-full-archive.test:32
+
+## Test that you can add a thin archive's contents to an existing full archive with 'L'
+# RUN: llvm-ar -q %tfull2.a %S/Inputs/d.txt
----------------
Also elsewhere with other comments.


================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1023-1024
     std::unique_ptr<object::Archive> Archive = std::move(ArchiveOrError.get());
-    if (Archive->isThin())
+    if (Thin && !Archive->isThin())
+      fail("cannot convert a regular archive to a thin one");
+
----------------
I've not looked at the full code paths, but at an educated guess, moving this code here will cause this error to occur in other cases which don't add new members, such as `t` operations? Is that desirable (I'm inclined to say so, but at least want to flag up the possibility of an issue)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123142