[PATCH] D48413: [llvm-objcopy] Add initial support for static libraries

James Henderson via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Jul 4 02:13:30 PDT 2018


jhenderson added inline comments.


================
Comment at: test/tools/llvm-objcopy/basic-archive-copy.test:1
+# RUN: yaml2obj %s > %t
+
----------------
jhenderson wrote:
> jakehehrlich wrote:
> > Can you add a test that has multiple objects in it? You can use one of the binaries from Input or you can duplicate the same binary.
> I'd like to see one more test case using a switch that results in some modification: the first member can be modified, but the second cannot (e.g. because it is a text file). This should result in an error and no manipulation of the first object either, I think (though I again could be persuaded that all members should be manipulated if possible).
Has this case been done? I'm expecting to see an extra case in strip-debug/strip-all, to show that something would indeed be changed.


================
Comment at: test/tools/llvm-objcopy/basic-archive-copy.test:37
+# RUN: llvm-ar crsT %t.thin.a %t
+# RUN: llvm-objcopy %t.thin.a %t2.thin.a
+# RUN: cat %t.thin.a | FileCheck %s --check-prefix=VERIFY-THIN-ARCHIVE
----------------
jhenderson wrote:
> Maybe worth checking that %t is the same as %t2?
Have I missed something? I don't see this being done anywhere.

For clarity, since the comment has moved, I was referring to checking the object file in the thin archive.


================
Comment at: test/tools/llvm-objcopy/basic-archive-copy.test:16-17
+# RUN: cmp %t.copy.a %t.a
+#INDEX-TABLE: Archive map
+#INDEX-TABLE-NEXT: foo in
+
----------------
Space after the '#' and please put a single blank line between the end of the RUNs and the check patterns, to make it a little clearer.

Ditto below for the other cases.


Repository:
  rL LLVM

https://reviews.llvm.org/D48413





More information about the llvm-commits mailing list