[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
+
----------------
sameerarora101 wrote:
> jhenderson wrote:
> > 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?
> Thanks, I was able to get rid of section contents as well. However, I still need the segment command or it throws an "invalid object file" error (perhaps because of  the string table below?). Updated it now.
Thanks. I don't know enough about Mach-O to be able to tell you why the object reading code won't handle it without segments. For now, I think this is fine.


================
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 =
----------------
smeenai wrote:
> jhenderson wrote:
> > You should probably avoid `auto` here. It's not obvious from the immediate context what the type is.
> I agree that it's not obvious, but the expanded type for this (and for iterators in general) is also extremely verbose :/ I thought the general guideline was to use `auto` for range based for loops for that reason. Perhaps we could stick with the `auto` in the loop and assign the members of the pair to variables in the loop? (It looks like we're only using the second member anyway.)
> I thought the general guideline was to use `auto` for range based for loops for that reason.

I wasn't aware that it was the guidance for all range based for loops? https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable seems to indicate `auto` where the context makes the type obvious, or the type is already abstracted away (usually applies for iterators).

The trouble is here that I have no idea that `NewMembers` is even a `DenseMap`, let alone a `DenseMap` of integers to vectors of ArchiveMembers. Maybe assigning the member to a local variable could help. Are you able to just use `std::pair` too, or does that not work? You can get rid of the `llvm::` prefixes everywhere here too.

There's some concern elsewhere about using `DenseMap` with integer keys, because -1/-2 are reserved values that result in assertions if used. Probably not that important, but maybe you would be better off using `unordered_map`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85334