[PATCH] D124895: [Object] Fix updating darwin archives

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-ar/macho-edit.test:1
+# REQUIRES: x86-registered-target
+
----------------
I don't think you need or want this.


================
Comment at: llvm/test/tools/llvm-ar/macho-edit.test:3
+
+## Make sure darwin specific timestamps are preserved when updating the archive
+# RUN: rm -rf %t; mkdir -p %t
----------------
Also below: comments should end with a full stop.


================
Comment at: llvm/test/tools/llvm-ar/macho-edit.test:4
+## Make sure darwin specific timestamps are preserved when updating the archive
+# RUN: rm -rf %t; mkdir -p %t
+# RUN: yaml2obj %p/Inputs/macho.yaml > %t/dup.o
----------------



================
Comment at: llvm/test/tools/llvm-ar/macho-edit.test:7
+
+## Create the archive with a duplicate object file to ensure that the timestamp workaround
+# RUN: llvm-ar --format=darwin crD %t/lib.a %t/dup.o %t/dup.o
----------------
Incomplete sentence?


================
Comment at: llvm/test/tools/llvm-ar/macho-edit.test:11
+
+## Inserting existing object is ignored but forces llvm-ar to rewrite the archive
+# RUN: llvm-ar crD %t/lib.a %t/dup.o
----------------
Actually, I'm pretty sure it's not ignored: `r` is replace, so it actively replaces the existing file (with an identical one). I could be wrong though - it's been a while since I've looked in depth at the archive file writing internals.


================
Comment at: llvm/test/tools/llvm-objcopy/MachO/archive-format.test:3
+
+## Make sure darwin specific timestamp logic is preserved in archives
+# RUN: rm -rf %t; mkdir -p %t
----------------



================
Comment at: llvm/test/tools/llvm-objcopy/MachO/archive-format.test:4
+## Make sure darwin specific timestamp logic is preserved in archives
+# RUN: rm -rf %t; mkdir -p %t
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %p/Inputs/macho.64.s -o %t/dup.o
----------------
`&&` is the more common pattern for linking these commands in tests from what I've seen. I think it's better too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124895