[PATCH] D83520: [llvm-libtool-darwin] Allow flattening archives

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-libtool-darwin/invalid-archive.test:4
+## Cannot read archive:
+# RUN: not llvm-libtool-darwin -static -o %t.lib %S/Inputs/invalid-archive.lib 2>&1 | \
+# RUN:   FileCheck %s --check-prefix=INVALID-ARCHIVE -DARCHIVE=%S/Inputs/invalid-archive.lib
----------------
Instead of using a separate Inputs file, you might want to consider using `echo` to create the input file on the fly. That has the advantage of keeping the test self-contained - you don't have to go looking elsewhere to understand what exactly is going on.


================
Comment at: llvm/test/tools/llvm-libtool-darwin/invalid-archive.test:9
+
+## Archive member not an object file:
+# RUN: rm -f %t.ar
----------------
I take it this behaviour is taken from the original libtool?


================
Comment at: llvm/test/tools/llvm-libtool-darwin/invalid-archive.test:12
+# RUN: touch %t.txt
+# RUN: llvm-ar crs %t.ar %t.txt
+# RUN: not llvm-libtool-darwin -static -o %t.lib %t.ar 2>&1 | \
----------------
Are you deliberately trying to add the archive symbol index here? That is done implicitly by llvm-ar anyway, so `cr` is sufficient.


================
Comment at: llvm/test/tools/llvm-libtool-darwin/invalid-archive.test:17
+## Do not recursively flatten archives:
+# RUN: rm -f %t.ar
+# RUN: yaml2obj %S/Inputs/input1.yaml -o %t-input1.o
----------------
This needs fixing (you are deleting the archive from the previous case, not the two archives from this case).


================
Comment at: llvm/test/tools/llvm-libtool-darwin/invalid-archive.test:19-20
+# RUN: yaml2obj %S/Inputs/input1.yaml -o %t-input1.o
+# RUN: llvm-ar crs %t.inner %t-input1.o
+# RUN: llvm-ar crs %t.outer %t.inner
+# RUN: not llvm-libtool-darwin -static -o %t.lib %t.outer 2>&1 | \
----------------
Ditto.


================
Comment at: llvm/test/tools/llvm-libtool-darwin/invalid-archive.test:27-31
+# RUN: rm -f %t.ar
+# RUN: yaml2obj %s -o %t.elf
+# RUN: llvm-ar crs %t.ar %t.elf
+# RUN: not llvm-libtool-darwin -static -o %t.lib %t.ar 2>&1 | \
+# RUN:   FileCheck %s --check-prefix=NOT-MACHO -DARCHIVE=%t.ar -DFILE=%basename_t.tmp.elf
----------------
Consider using a different archive file name here, so that you don't overwrite the archive from the earlier case. This can be useful in some debugging situations.


================
Comment at: llvm/test/tools/llvm-libtool-darwin/valid-archive.test:1
+## This test checks that an archive is flattened correctly.
+
----------------
I'd rename this test `archive-flattening.test`. You should also fold in the invalid cases into it, as there's no need to keep them separate.


================
Comment at: llvm/test/tools/llvm-libtool-darwin/valid-archive.test:7
+# RUN: rm -f %t.ar
+# RUN: rm -f %t.lib
+
----------------
What's the reasoning for deleting `%t.lib` explicitly here?


================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:98
+    Error Err = Error::success();
+    for (auto &Child : Lib.children(Err))
+      if (Error E = addChildMember(Members, Child))
----------------
Don't use `auto` here. It's not obvious what the type is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83520