[Lldb-commits] [PATCH] D113608: [lldb] Simplify specifying of platform supported architectures

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere accepted this revision.
JDevlieghere added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: lldb/source/Target/Platform.cpp:1217-1218
+std::vector<ArchSpec>
+Platform::CreateArchList(llvm::Triple::OSType os,
+                         llvm::ArrayRef<llvm::Triple::ArchType> archs) {
+  std::vector<ArchSpec> list;
----------------
labath wrote:
> JDevlieghere wrote:
> > In PlatformDarwin we set the vendor too. Could we have this take an optional OSType and Vendor maybe that sets the ones that are not None? That would also make migrating PlatformDarwin easier where we currently share the code and don't set the OS. Later we could move this out of PlatformDarwin into the child classes and have the OS set correctly. 
> Umm.. maybe? I don't think this function is set in stone, but the nice thing about it being a helper function is that it's usage is completely optional. I haven't tried porting the darwin platforms yet, but, from a cursory glance I did see that they have a lot more complex usage patterns:
> - they pass subarchitectures as well, so we'd either also need to have an Optional subarch field, or switch to passing architectures as strings (I mean, I suppose I could switch to strings now, but it feels more correct to work with named constants)
> - a few of them also pass environments 
> - and some don't even have matching OS fields (`x86_64-apple-macosx` and `arm64-apple-ios` in the same list)
> 
> So, I'd leave it to those patches to determine whether it's better to extend this function or create a new one.
Okay, I'm happy to modify the helper when I start using it for the darwin platfomrs. I think at least some of the complexity/inconsistencies is purely historic such as the one where we were specifying an OS in the list of supported architectures in PlatformDarwin. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113608