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