[PATCH] D63735: [MachOObjectFile]Added Valid Architecture Function

Mon Jul 1 17:11:38 PDT 2019

smeenai added inline comments.

================
Comment at: llvm/include/llvm/Object/MachO.h:574
   static bool isValidArch(StringRef ArchFlag);
+  static ArrayRef<StringRef> getValidArchs();
   static Triple getHostArch();
----------------
smeenai wrote:
> compnerd wrote:
> > smeenai wrote:
> > > smeenai wrote:
> > > > mtrent wrote:
> > > > > Super duper nitty nit: consider "getKnownArchs" instead of "getValidArchs". I'm wrestling with the idea of what makes an arch "valid", considering the list supported here is only the subset of archs that llvm knows about. E.g., is m68k an invalid Mach-O file? And so on. I readily concede this is the most pedantic of concerns.
> > > > > 
> > > > > I can see how an argument can be invalid, and so have no concerns over llvm-lipo's validateArchitectureName function name.
> > > > We went with "valid" because that's the terminology the existing `isValidArch` was using. Should we rename that to `isKnownArch` as well?
> > > Ping @mtrent on the renaming question :)
> > I think maybe `Supported` is more appropriate.  PPC should be a known valid arch as IIRC even though it is not currently supported by the interface.
> I like `Supported`, but for PowerPC specifically I'll note that the current `isValidArch` does return true for it. If we renamed it to `isSupportedArch` I'm not sure if Darwin PPC is still considered supported...
Thoughts on the best terminology here?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63735