[PATCH] D67700: [Object] Extend MachOUniversalBinary::getObjectForArch

[Alexander Shaposhnikov via Phabricator via llvm-commits]

alexshap marked an inline comment as done.
alexshap added inline comments.


================
Comment at: include/llvm/Object/MachOUniversal.h:162
 
-  Expected<std::unique_ptr<MachOObjectFile>>
+  Expected<ObjectForArch>
   getObjectForArch(StringRef ArchName) const;
----------------
mtrent wrote:
> compnerd wrote:
> > `ObjectForArch`?  That seems like an odd type.  Why not use `Binary` if you are looking for a common base type for an object file and an archive?
> IIUC, the idea is to restrict the kind of things that one might reasonably find in a universal file. For example, constrain the output from getObjectForArch to be either a Mach-O file or an Archive. Binary would imply you would have to deal with the possibility that a universal file might contain another universal file, or a non-binary file such as a plist, ELF binaries, or literally anything. 
> 
> Because people can make universal files themselves (it's a very simple file format), we find sometimes people abuse the format and shove weird stuff in there. Sometimes it's something "wrong-but-benign" like shoving an macOS-compiled simulator framework alongside an iOS-compiled framework. Sometimes it's something "wrong-just-wrong". 
> 
> ObjectForArch is an existing class meant to put the burden of checking types on MachOUniversal.cpp. Returning Binary would put the burden of checking types onto each caller. I agree its a weird type name to expose to callers. 
> 
> A reasonable alternative would be to add "getArchiveForArch" alongside "getObjectForArch" and then provide a third "isObject / isArchive" accessor so callers knew which method to call. That's probably the same order of patch as this change.
@compnerd 
Just wanted to add  (I also mentioned this in the description), that I was looking at
the implementation of MachOUniversalBinary::ObjectForArch::getAsObjectFile / MachOUniversalBinary::ObjectForArch::getAsArchive. The thing is that the both methods create
MachOObjectFile / Archive instances on "the fly" using ObjectFile::createMachOObjectFile / Archive::create respectively. The both methods can return Error for multiple reasons. 
Unpacking Expected<MachOObjectFile> and Expected<Archive> and picking the "right" error didn't look good to me, that's why preferred the current approach.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D67700