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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
