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

_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to