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

[Pavel Labath via Phabricator via lldb-commits]

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