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

[Sameer Arora via Phabricator via llvm-commits]

sameerarora101 marked 6 inline comments as done.
sameerarora101 added inline comments.


================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:123
 
+static uint64_t getCPUID(uint32_t CPUType, uint32_t CPUSubtype) {
+  switch (CPUType) {
----------------
smeenai wrote:
> sameerarora101 wrote:
> > jhenderson wrote:
> > > Looks to me like this function is only partially tested? What about the other three CPUs and the default case?
> > Ok also added test for `CPU_TYPE_ARM64`,  `CPU_TYPE_X86_64` and the default case. `CPU_TYPE_ARM64_32` does not have a subtype so I didn't add a test for it.
> Can you have two files with a CPUType that falls under the default case but different CPUSubtypes, and confirm that they get put in the same slice of the output file?
Besides the 4 non-default cases below, the file `llvm/lib/Object/MachOObjectFile.cpp` supports the following CPU types (which will fall under default case):
- `MachO::CPU_TYPE_I386`
- `MachO::CPU_TYPE_POWERPC`
- `MachO::CPU_TYPE_POWERPC64`

However, for all these default CPU types, there is just a **single** subtype:
- `MachO::CPU_SUBTYPE_I386_ALL`
- `MachO::CPU_SUBTYPE_POWERPC_ALL`
- `MachO::CPU_SUBTYPE_POWERPC_ALL`

Thus, I am unable to have two files having the same CPU type that falls under default case but have different Subtypes :( 
Any other way I should test this?


================
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:
> 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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85334