labath added inline comments.
================
Comment at: lldb/include/lldb/Target/Platform.h:330-331
+ /// NB: This implementation is mutually recursive with
+ /// GetSupportedArchitectureAtIndex. Subclasses should implement at least one
+ /// of them.
+ virtual std::vector<ArchSpec> GetSupportedArchitectures();
----------------
JDevlieghere wrote:
> Do you expect any platform to implement both?
Not really, though it wouldn't necessarily be an error if they did. I'll just
remove the "at least" part.
================
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;
----------------
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.
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
