[Lldb-commits] [PATCH] D84537: [lldb/AppleSimulator] Always provide a -simulator environment

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added inline comments.


================
Comment at: lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.cpp:270
+    static const ArchSpec platform_arch(
+        HostInfo::GetArchitecture(HostInfo::eArchKind64));
+    arch = platform_arch;
----------------
I guess I should really know this... does this construct create a static initializer? If yes, isn't that something we generally want to avoid in LLVM?

I checked: HostInfo::GetArchitecture() caches the result anyway, so there is no need to cache it again here.


================
Comment at: lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.h:59
+  llvm::Triple::OSType m_os_type = llvm::Triple::UnknownOS;
+  llvm::ArrayRef<llvm::StringRef> m_supported_triples = {};
+
----------------
If the StringRef is supposed to be a triple, we might want to store an array of llvm::Triple instead? They are basically std::strings.


================
Comment at: lldb/source/Plugins/Platform/MacOSX/PlatformAppleTVSimulator.cpp:81
+    case llvm::Triple::aarch64:
     case llvm::Triple::x86_64: {
       const llvm::Triple &triple = arch->GetTriple();
----------------
at some point we should factor out all the common code in the simulator platforms ...


================
Comment at: lldb/source/Plugins/Platform/MacOSX/PlatformAppleTVSimulator.cpp:159
+  static const llvm::StringRef supported_triples[] = {
+      "x86_64-apple-tvos-simulator",
+  };
----------------
I'm probably missing something: Instead of declaring the static list of supported triples here and then manually adding the host architecture, why not create the list on-the-fly in GetSupportedArchitectureAtIndex()? Is `m_supported_triples` used directly somewhere?

Something like:

```
switch (i) {
#ifdef __APPLE__
#if __arm64__
case 0:  return  "arm64-apple-tvos-simulator";
case 1:  return  "x86_64-apple-tvos-simulator";
case 2: return "x86_64h-apple-tvos-simulator",
  };
#else
if (is_haswell)
switch (i) {
case 0: return "x86_64h-apple-tvos-simulator";
case 1: return "x86_64-apple-tvos-simulator";
}
else  return "x86_64-apple-tvos-simulator"
```




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84537/new/

https://reviews.llvm.org/D84537