[PATCH] D85334: [llvm-libtool-darwin] Support universal outputs

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-libtool-darwin/universal-output.test:4-5
+
+# RUN: yaml2obj %s --docnum=1 -o %t.armv6
+# RUN: yaml2obj %s --docnum=2 -o %t.armv7
+
----------------
Rather than having two near-identical YAML blobs, use the `-D` option to pull out the different bits:

```
# RUN: yaml2obj %s --docnum=1 -o %t.armv6 -DSUBTYPE=0x6 ...
# RUN: yaml2obj %s --docnum=2 -o %t.armv7 -DSUBTYPE=0x9 ...
```

There's a lot of extra noise in the two, which I suspect you can get rid of with a bit of effort. You don't need to have the YAML directly represent a real object you've made, as long as it's sufficient to trigger the code paths you need. For example, do you actually need the segment and section contents?


================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:123
 
+static uint64_t getCPUID(uint32_t CPUType, uint32_t CPUSubtype) {
+  switch (CPUType) {
----------------
Looks to me like this function is only partially tested? What about the other three CPUs and the default case?


================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:354
+    SmallVector<OwningBinary<Archive>, 2> OutputBinaries;
+    for (const auto &M : NewMembers) {
+      Expected<std::unique_ptr<MemoryBuffer>> OutputBufferOrErr =
----------------
You should probably avoid `auto` here. It's not obvious from the immediate context what the type is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85334