[PATCH] D64668: [llvm-lipo] Implement -info

[Saleem Abdulrasool via Phabricator via llvm-commits]

compnerd added inline comments.


================
Comment at: llvm/tools/llvm-lipo/llvm-lipo.cpp:301
+static void printInfo(ArrayRef<OwningBinary<Binary>> InputBinaries) {
+  std::string UniversalOutputBuffer;
+  raw_string_ostream UniversalOutputStream(UniversalOutputBuffer);
----------------
anushabasana wrote:
> alexshap wrote:
> > alexshap wrote:
> > > khm, the same effect (grouping universal/regular files) could be achieved a bit simpler, what would you say to the following (no need for std::string, raw_string_ostream, ThinOutputBuffer, ThinOutputStream, UniversalOutputBuffer, UniversalOutputStream, etc)
> > >      
> > >     for (const auto &IB : InputBinaries)
> > >         if (IB.getBinary()->isMachOUniversalBinary()) {
> > >              outs() << "Architectures in the fat file: " << Binary->getFileName() 
> > >                        << " are: ";
> > >              printBinaryArchs(IB.getBinary(), outs());
> > >         }
> > >      for (const auto &IB : InputBinaries)
> > >         if (IB.getBinary()->isMachO) {
> > >              outs() << "Non-fat file: " << IB.getBinary()->getFileName()
> > >                        << " is architecture: ";
> > >              printBinaryArchs(IB.getBinary(), outs());
> > >         }            
> > cc: @mtrent 
> > 1. The wording (especially in the second case) appears to be very weird, maybe it's worth to change it and make it more human-readable ? (I suspect `-info` is not really widely used in scripts, so probably this incompatibility might not be a big issue).
> > 2. Maybe we don't even need to group anything here (for the same reasons)
> I could remove the string and OS for Universal files and just pass in outs() to cut down there.
> I originally had 2 for loops but I thought the multiple OS would be better so we would only have to do one pass over the input binaries.
> I'm not sure what is more legible, I don't have a strong preference.
@alexshap - I think that I would rather that we go with stronger claims:

```
for (const auto &IB : InputBinaries) {
  Binary *B = IB.getBinary();
  assert(B->isMachO() && "expected MachO binary");
  outs() << "Non-fat file: " << B->getFileName() << " is architecture: ";
  printBinaryArchs(B, outs());
}
```


================
Comment at: llvm/tools/llvm-lipo/llvm-lipo.cpp:547
+    printInfo(InputBinaries);
+    break;
   case LipoAction::ThinArch:
----------------
alexshap wrote:
> compnerd wrote:
> > Is there validation being applied here?  I think that you should add a test case for invalid input as well (that is, pass ELF files to `lipo` and ensure that we handle that gracefully (right now, it feels like we may just abort?).
> if I'm not mistaken `readInputBinaries` takes care of it, otherwise this would be reimplemented N times where N is the number of supported actions.  Having an extra test of `-info` would not hurt though.
Ah, okay, I was trying to see where that was called, I guess I just failed to see it.  Yes, a test would be good still.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64668