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

[Michael Trent via Phabricator via llvm-commits]

mtrent added inline comments.


================
Comment at: include/llvm/Object/MachOUniversal.h:162
 
-  Expected<std::unique_ptr<MachOObjectFile>>
+  Expected<ObjectForArch>
   getObjectForArch(StringRef ArchName) const;
----------------
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.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D67700