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

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.


================
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:
> > 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. 
> Cool. Does that mean I can count on you do convert the darwin platforms? :)
> 
> (I was going to try doing it myself, but I have basically no way of checking whether my modifications break anything.)
Yup, although I can't commit to a timeframe :-) For validation you'd need to run the "on device" test suite and there's no real way for non-Apple people to do that, but if you get to it before me I'm more than happy to apply the patch locally and try it out. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113608