[PATCH] D85740: Universal MachO: support LLVM IR objects

[Alexander Shaposhnikov via Phabricator via llvm-commits]

alexshap added inline comments.


================
Comment at: llvm/lib/Object/MachOUniversalWriter.cpp:150
+      }
+      continue;
     }
----------------
not particularly important, but I'd probably convert "if" + "continue" into
   if ( ... ) { ... } else if ( ... ) { ... } else { ... }

(it seems to be more readable)


================
Comment at: llvm/lib/Object/MachOUniversalWriter.cpp:206
 
-  Slice ArchiveSlice = Slice(*(FO.get()), FO->is64Bit() ? 3 : 2);
+  assert(!(MFO && IRFO));
+
----------------
perhaps, given the check above this assert is not super useful
if you decide to keep it  then we need to add a string message
   assert (.... && "....")


================
Comment at: llvm/lib/Object/MachOUniversalWriter.cpp:215
+  // For IR objects
+  auto ArchiveSliceOrErr = Slice::create(IRFO.get(), 0);
+  if (!ArchiveSliceOrErr) {
----------------
nit:
1. auto -> explicit type (since it's not clear from the right-hand side),
2. for a single-line "if" braces are not necessary.

(and the same applies to lines 216 - 219)


================
Comment at: llvm/tools/llvm-lipo/llvm-lipo.cpp:128
 
+static Slice irSlice(const IRObjectFile *IRO, StringRef File, unsigned Align) {
+  Expected<Slice> IROrErr = Slice::create(IRO, Align);
----------------
what would you say to the following renaming:
    irSlice -> createSliceFromIR
    archiveSlice -> createSliceFromArchive


================
Comment at: llvm/tools/llvm-lipo/llvm-lipo.cpp:128
 
+static Slice irSlice(const IRObjectFile *IRO, StringRef File, unsigned Align) {
+  Expected<Slice> IROrErr = Slice::create(IRO, Align);
----------------
alexshap wrote:
> what would you say to the following renaming:
>     irSlice -> createSliceFromIR
>     archiveSlice -> createSliceFromArchive
I'm wondering if we really need to pass the name of the file here - can't it be extracted from the first argument (IRO->getFileName()) ?

(if yes - then probably it can be removed from archiveSlice as well)

P.S. https://llvm.org/doxygen/MachOUniversal_8cpp_source.html#l00078 


================
Comment at: llvm/tools/llvm-lipo/llvm-lipo.cpp:399
+        consumeError(MachOObjOrError.takeError());
+        auto SliceOrErr = Slice::create(IROrError->get(), O.getAlign());
+        if (!SliceOrErr) {
----------------
nit: explicit type


================
Comment at: llvm/tools/llvm-lipo/llvm-lipo.cpp:566
+          consumeError(BinaryOrError.takeError());
+          auto Slice =
+              irSlice(static_cast<IRObjectFile *>(IROrError.get().get()),
----------------
nit: explicit type


================
Comment at: llvm/tools/llvm-lipo/llvm-lipo.cpp:581
+      // Original Apple's lipo set the alignment to 0
+      auto SliceOrErr = Slice::create(IRO, 0);
+      if (!SliceOrErr) {
----------------
nit: explicit type


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85740