[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:76
+
+# ARCHS-CPU: Architectures in the fat file: [[FILE]] are: {{(armv6|ppc) (armv6|ppc)}}
+
----------------
Better would be `{{(armv6 ppc)|(ppc armv6)}}`. The current pattern could match `armv6 armv6` for example.


================
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 =
----------------
sameerarora101 wrote:
> jhenderson wrote:
> > 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`?
> ok, updated it to use `unordered_map`? How does this look now?
I think you can drop the `const` from the `uin64_t`, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85334