[Lldb-commits] [PATCH] D39387: Invert ArchSpec<->Platform dependency

[Zachary Turner via Phabricator via lldb-commits]

zturner added inline comments.


================
Comment at: include/lldb/Host/HostInfoBase.h:85
+  //---------------------------------------------------------------------------
+  /// If the triple contains not specify the vendor, os, and environment parts,
+  /// we "augment" these using information from the host and return the
----------------
s/contains/does/


================
Comment at: include/lldb/Target/Platform.h:524
+  //---------------------------------------------------------------------------
+  ArchSpec GetAugmentedArchSpec(llvm::StringRef triple);
+
----------------
Should this function be marked `const`?

Another possible interface would be `void AugmentArchSpec(ArchSpec &spec);`  


================
Comment at: source/API/SBDebugger.cpp:698
+    ArchSpec arch = Platform::GetAugmentedArchSpec(
+        m_opaque_sp->GetPlatformList().GetSelectedPlatform().get(), arch_name);
     TargetSP target_sp(
----------------
Is `arch_name` here actually a triple?  If so, is it worth renaming this variable?


================
Comment at: source/API/SBInstruction.cpp:263
   if (inst_sp && triple) {
-    lldb_private::ArchSpec arch(triple, NULL);
-    return inst_sp->DumpEmulation(arch);
+    return inst_sp->DumpEmulation(HostInfo::GetAugmentedArchSpec(triple));
   }
----------------
Why does this one use `HostInfo` instead of `Platform`?


================
Comment at: source/Core/ArchSpec.cpp:889
 
-bool ArchSpec::SetTriple(llvm::StringRef triple, Platform *platform) {
-  if (triple.empty()) {
-    Clear();
-    return false;
-  }
-  if (ParseMachCPUDashSubtypeTriple(triple, *this))
-    return true;
-
-  if (triple.startswith(LLDB_ARCH_DEFAULT)) {
-    // Special case for the current host default architectures...
-    if (triple.equals(LLDB_ARCH_DEFAULT_32BIT))
-      *this = HostInfo::GetArchitecture(HostInfo::eArchKind32);
-    else if (triple.equals(LLDB_ARCH_DEFAULT_64BIT))
-      *this = HostInfo::GetArchitecture(HostInfo::eArchKind64);
-    else if (triple.equals(LLDB_ARCH_DEFAULT))
-      *this = HostInfo::GetArchitecture(HostInfo::eArchKindDefault);
-    return IsValid();
-  }
-
-  ArchSpec raw_arch(triple);
-
+bool ArchSpec::ContainsMoreThanArch(llvm::StringRef triple) {
   llvm::Triple normalized_triple(llvm::Triple::normalize(triple));
----------------
Should this function be marked `const`?


================
Comment at: source/Host/common/HostInfoBase.cpp:257-261
+  if (ArchSpec::ContainsMoreThanArch(triple))
+    return ArchSpec(triple);
+
+  llvm::Triple normalized_triple(llvm::Triple::normalize(triple));
+  llvm::Triple host_triple(llvm::sys::getDefaultTargetTriple());
----------------
I don't *think* this is very performance sensitive, but it's unfortunate that this ends up calling `normalize` twice.  If there's a clean way to re-write it that would probably be nice.


================
Comment at: source/Target/Platform.cpp:975-976
+    return ArchSpec();
+  if (ArchSpec::ContainsMoreThanArch(triple))
+    return ArchSpec(triple);
+
----------------
Same here, this method ends up normalizing twice, once inside of `ContainsMoreThanArch` and once inside of this function.


================
Comment at: source/Target/Platform.cpp:984
+  llvm::Triple normalized_triple(llvm::Triple::normalize(triple));
+  if (compatible_arch.IsValid()) {
+    const llvm::Triple &compatible_triple = compatible_arch.GetTriple();
----------------
Can you early-return here?


================
Comment at: source/Target/Platform.cpp:986-991
+    if (normalized_triple.getVendorName().empty())
+      normalized_triple.setVendor(compatible_triple.getVendor());
+    if (normalized_triple.getOSName().empty())
+      normalized_triple.setOS(compatible_triple.getOS());
+    if (normalized_triple.getEnvironmentName().empty())
+      normalized_triple.setEnvironment(compatible_triple.getEnvironment());
----------------
Are these cases even possible?  Why would the vendor and os ever be empty?  I thought only the environment could be empty.


https://reviews.llvm.org/D39387