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

[Sameer Arora via Phabricator via llvm-commits]

sameerarora101 marked an inline comment as done.
sameerarora101 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)}}
+
----------------
jhenderson wrote:
> Better would be `{{(armv6 ppc)|(ppc armv6)}}`. The current pattern could match `armv6 armv6` for example.
Ah yes, you are right. Thanks!


================
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 =
----------------
jhenderson wrote:
> 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?
I guess not. If I do that, it doesn't compile and throws the following error:
```
/home/sameerarora101/local/llvm-project/llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:354:10:   required from here
/usr/include/c++/8/bits/stl_construct.h:75:7: error: use of deleted function ‘llvm::NewArchiveMember::NewArchiveMember(const llvm::NewArchiveMember&)’
     { ::new(static_cast<void*>(__p)) _T1(std::forward<_Args>(__args)...); }

```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85334