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

[Adrien Guinet via Phabricator via llvm-commits]

aguinet added a comment.

Thanks @alexshap for your review and comments! Answers are inline.

I will try and push a new diff with the suggested modifications during the day.



================
Comment at: llvm/lib/Object/MachOUniversalWriter.cpp:150
+      }
+      continue;
     }
----------------
alexshap wrote:
> not particularly important, but I'd probably convert "if" + "continue" into
>    if ( ... ) { ... } else if ( ... ) { ... } else { ... }
> 
> (it seems to be more readable)
Okay will fix with new diff!


================
Comment at: llvm/lib/Object/MachOUniversalWriter.cpp:206
 
-  Slice ArchiveSlice = Slice(*(FO.get()), FO->is64Bit() ? 3 : 2);
+  assert(!(MFO && IRFO));
+
----------------
alexshap wrote:
> 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 (.... && "....")
Indeed. I put it during development to make sure I didn't make any mistake, but I will remove it.


================
Comment at: llvm/lib/Object/MachOUniversalWriter.cpp:215
+  // For IR objects
+  auto ArchiveSliceOrErr = Slice::create(IRFO.get(), 0);
+  if (!ArchiveSliceOrErr) {
----------------
alexshap wrote:
> 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)
Ack will fix with new diff


================
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:
> 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 
Probably. I will test and let you know :)

No problem for me for the renaming you propose, will put it in the new diff.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85740